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.

Sign up for GitHub

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

Jump to bottom

Added the ability to append an InferenceData object to an existing netCDF file #2227

Merged
merged 7 commits into from
Jun 10, 2023
Merged

Added the ability to append an InferenceData object to an existing netCDF file #2227

merged 7 commits into from
Jun 10, 2023

Conversation

varchasgopalaswamy
Copy link
Contributor

@varchasgopalaswamy varchasgopalaswamy commented Mar 23, 2023 •
edited by OriolAbril
Loading

Checklist

  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

📚 Documentation preview 📚: https://arviz--2227.org.readthedocs.build/en/2227/

OriolAbril
OriolAbril reviewed Apr 3, 2023
View reviewed changes
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. Thanks for the 2nd PR!

It looks like the PR adds both the option to append to an existing netCDF file and to read/write InferenceData as lower level groups within a user-given base group. Is that right?

@varchasgopalaswamy
Copy link
Contributor Author

Sorry for the late review. Thanks for the 2nd PR!

It looks like the PR adds both the option to append to an existing netCDF file and to read/write InferenceData as lower level groups within a user-given base group. Is that right?

Yeah, that's the idea!

OriolAbril
OriolAbril reviewed Apr 3, 2023
View reviewed changes
arviz/data/inference_data.py Outdated Show resolved Hide resolved
arviz/data/inference_data.py Outdated Show resolved Hide resolved
arviz/data/inference_data.py Outdated Show resolved Hide resolved
arviz/data/inference_data.py Show resolved Hide resolved
arviz/data/inference_data.py Outdated Show resolved Hide resolved
OriolAbril
OriolAbril reviewed Apr 5, 2023
View reviewed changes
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I think the code is good to go now, thanks! Can you add a test too in https://github.com/arviz-devs/arviz/blob/main/arviz/tests/base_tests/test_data.py for this new arguments?

CHANGELOG.md Outdated Show resolved Hide resolved
arviz/data/inference_data.py Outdated Show resolved Hide resolved
OriolAbril
OriolAbril reviewed Apr 11, 2023
View reviewed changes
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Last minor nit, I think it will be ready to merge after this round

arviz/tests/base_tests/test_data.py Outdated
@@ -1294,10 +1294,11 @@ def test_io_function(self, data, eight_schools_params):
os.remove(filepath)
assert not os.path.exists(filepath)

@pytest.mark.parametrize("base_group", ['/','/test_group'])
Copy link
Member

Choose a reason for hiding this comment

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

Is the initial / in the group name necessary? If so we should document it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so... not unless the NetCDF interface library requires it.

@varchasgopalaswamy
Copy link
Contributor Author

@OriolAbril is there anything else needed to merge the PR?

OriolAbril
OriolAbril approved these changes Jun 10, 2023
View reviewed changes
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

fixed some black complaints, should be good to go now. Sorry about the slow review cadence, thank you so much for all the PRs!

@codecov
Copy link

codecov bot commented Jun 10, 2023

Codecov Report

Merging #2227 (b1a6ff5) into main (88f7e2a) will decrease coverage by 0.01%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##             main    #2227      +/-   ##
==========================================
- Coverage   87.91%   87.90%   -0.01%     
==========================================
  Files         120      120              
  Lines       12424    12432       +8     
==========================================
+ Hits        10922    10928       +6     
- Misses       1502     1504       +2
Impacted Files Coverage Δ
arviz/data/inference_data.py 82.31% <84.61%> (-0.08%) ⬇️

@OriolAbril OriolAbril merged commit 353508d into arviz-devs:main Jun 10, 2023
@varchasgopalaswamy
Copy link
Contributor Author

No problem - thank you for the great package! Any idea when there might be a new release? I'd like to stop pinning my software to a github ref!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OriolAbril OriolAbril OriolAbril approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@varchasgopalaswamy @OriolAbril