Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HERD Updates #968

Merged
merged 50 commits into from
Dec 13, 2023
Merged

HERD Updates #968

merged 50 commits into from
Dec 13, 2023

Conversation

mavaylon1
Copy link
Contributor

@mavaylon1 mavaylon1 commented Oct 18, 2023

Motivation

What was the reasoning behind this change? Please explain the changes briefly.

  • HERD should resolve the reuse of keys when it is being associated with the same object. This will allow us to bulk add but also gives the user to still manually choose a key that exists if they want to use that key later on.
  • HERD should do checks before populating the tables. Currently, the checks are spread out (some we may not be able to move so TBD). This leads to some tables being populated and some are not when an error is thrown. If the checks can't be moved as easily as I think, then we would need to have something that removes what was added. I prefer the former to the latter.
  • HERD should rename add_ref_termset to maybe "add_ref_container" since it is only called in the I/O for write.
  • HERD should have a add_ref_termset that actually uses termsets instead of entity id or uri. We had this prior but this changed when we introduced the wrapper. This would bring the essence of that back.
  • HERD should resolve existing entities with a warning and use the uri of current entity and not the parameter.
  • Add tests for coverage
  • Update tutorial to account for add_ref_termset

How to test the behavior?

Show how to reproduce the new behavior (can be a bug fix or a new feature)

Unit Tests

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running ruff from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (41fabd4) 88.57% compared to head (ada2e30) 88.38%.

Files Patch % Lines
src/hdmf/common/resources.py 96.58% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #968      +/-   ##
==========================================
- Coverage   88.57%   88.38%   -0.20%     
==========================================
  Files          45       45              
  Lines        9509     9565      +56     
  Branches     2704     2720      +16     
==========================================
+ Hits         8423     8454      +31     
- Misses        764      784      +20     
- Partials      322      327       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mavaylon1 mavaylon1 self-assigned this Oct 18, 2023
@mavaylon1
Copy link
Contributor Author

Fix #961

@mavaylon1
Copy link
Contributor Author

This PR has grown to cover multiple items.

  • add_ref_termset
  • resolving keys within the same object
  • resolving entities to ignore new a uri

I remembered a question about get_key and I want to get better at limiting the scope of PRs. I will continue off of the original PR: #964
The idea is that get_key should:

  • If no key is found return None instead of raising a ValueError
  • If multiple matching keys are found, then return a list of Key objects rather than raising a ValueError
    I don't think changing to None is more informative. I like the fact the error tells you why. As for the second one, I think that's fine.

@mavaylon1 mavaylon1 requested a review from rly November 1, 2023 15:40
@mavaylon1 mavaylon1 marked this pull request as ready for review November 1, 2023 15:40
@mavaylon1
Copy link
Contributor Author

@rly A note on your comment of checking that the container is in the file is a good idea. It avoids bad references. I added that change and it would require some major reworking of the test suite. I'd like to make this into a PR that includes the "HDMF_File" as it would go in line with updating the tests. I added the actual code already and a test, these will be flushed out in the next PR. This would also include updates to the doc.

I don't want to manually add the parent in the tests and the documentation then remove them with the HERDFile update.

@mavaylon1 mavaylon1 requested a review from rly December 12, 2023 18:49
src/hdmf/common/resources.py Outdated Show resolved Hide resolved
@mavaylon1 mavaylon1 merged commit a270a14 into dev Dec 13, 2023
28 checks passed
@mavaylon1 mavaylon1 deleted the herd_update branch December 13, 2023 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants