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

ExternalResources I/O #895

Merged
merged 10 commits into from
Jul 9, 2023
Merged

ExternalResources I/O #895

merged 10 commits into from
Jul 9, 2023

Conversation

mavaylon1
Copy link
Contributor

@mavaylon1 mavaylon1 commented Jul 7, 2023

Motivation

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

This is to add logic to support ExternalResources on HDMFIO and HDF5IO. This was set up due to pynwb (https://github.com/NeurodataWithoutBorders/pynwb/actions/runs/5488375497?pr=1684). The update simply sets up external resources in the corresponding init methods.

How to test the behavior?

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

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 Jul 7, 2023

Codecov Report

Patch coverage: 94.73% and no project coverage change.

Comparison is base (0c01dd7) 88.23% compared to head (93a2c01) 88.24%.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #895   +/-   ##
=======================================
  Coverage   88.23%   88.24%           
=======================================
  Files          45       45           
  Lines        9232     9247   +15     
  Branches     2638     2641    +3     
=======================================
+ Hits         8146     8160   +14     
  Misses        768      768           
- Partials      318      319    +1     
Impacted Files Coverage Δ
src/hdmf/backends/io.py 96.10% <94.11%> (-0.68%) ⬇️
src/hdmf/backends/hdf5/h5tools.py 82.36% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@oruebel
Copy link
Contributor

oruebel commented Jul 7, 2023

Overall, this looks good to me. I'm wondering whether this should also include the reading of the ExternalResources file in HDMFIO.read:

if self.external_resources is not None:
            er_read = ExternalResources.from_norm_tsv(path=self.external_resources)
            file.link_resources(er_read)

@mavaylon1
Copy link
Contributor Author

mavaylon1 commented Jul 7, 2023

Overall, this looks good to me. I'm wondering whether this should also include the reading of the ExternalResources file in HDMFIO.read:

if self.external_resources is not None:
            er_read = ExternalResources.from_norm_tsv(path=self.external_resources)
            file.link_resources(er_read)

I did the local import, but without the link_resources since that exists only on ExternalResourcesManager.

@mavaylon1 mavaylon1 marked this pull request as ready for review July 7, 2023 20:49
@mavaylon1
Copy link
Contributor Author

I see there's a test missing. I'll add it.

@mavaylon1
Copy link
Contributor Author

mavaylon1 commented Jul 9, 2023

Given that py3.7 has made it to the special farm upstate, can we ignore the conda 3.7 test issue? @rly

We should also make the update to remove it on a new ticket.

@mavaylon1
Copy link
Contributor Author

@oruebel could you give this a gander so we can do a release and then merge the pynwb PR.

src/hdmf/backends/io.py Outdated Show resolved Hide resolved
Co-authored-by: Oliver Ruebel <[email protected]>
@mavaylon1 mavaylon1 requested a review from oruebel July 9, 2023 19:24
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@mavaylon1 mavaylon1 merged commit b89679d into dev Jul 9, 2023
@rly rly deleted the er_io branch July 10, 2023 23:45
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.

2 participants