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

[WIP] Appropriately handle skipped frames #44

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

[WIP] Appropriately handle skipped frames #44

wants to merge 27 commits into from

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Jan 20, 2025 •
edited by hannahbaumann
Loading

Fixes #41

TODO:

  • upload relevant skipped NetCDF files somewhere
  • write more tests

@IAlibay IAlibay mentioned this pull request Jan 20, 2025
Draft
9 tasks
hannahbaumann
hannahbaumann requested changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@hannahbaumann hannahbaumann left a comment

Choose a reason for hiding this comment

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

Thanks @IAlibay , I know this is still [WIP] , so just some preliminary comments, but otherwise lgtm!

src/openfe_analysis/reader.py Outdated Show resolved Hide resolved
src/openfe_analysis/reader.py Outdated Show resolved Hide resolved
src/openfe_analysis/utils/multistate.py Outdated Show resolved Hide resolved
src/openfe_analysis/reader.py Outdated Show resolved Hide resolved
src/openfe_analysis/utils/multistate.py Outdated Show resolved Hide resolved
src/openfe_analysis/reader.py Show resolved Hide resolved
@hannahbaumann hannahbaumann self-assigned this Jan 21, 2025
@IAlibay
Copy link
Member Author

IAlibay commented Jan 23, 2025

@hannahbaumann if you could take over here, especially with the tests that would be grand!

The figshare with the new files is here: https://figshare.com/articles/dataset/OpenFE_1_3_0beta_example_results_files/28263203

@IAlibay IAlibay added this to the v0.3.0 milestone Jan 23, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 23, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.17949% with 10 lines in your changes missing coverage. Please review.

Project coverage is 71.81%. Comparing base (f3a37f0) to head (1b9cd3c).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/openfe_analysis/utils/multistate.py 70.37% 8 Missing ⚠️
src/openfe_analysis/reader.py 94.11% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main      #44       +/-   ##
===========================================
+ Coverage   60.06%   71.81%   +11.74%     
===========================================
  Files           6        7        +1     
  Lines         298      337       +39     
===========================================
+ Hits          179      242       +63     
+ Misses        119       95       -24

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

@hannahbaumann
Copy link
Contributor

The tests are quite flaky for some reason, I wonder if it's related to the size of the original NetCDF file. I added a pytest.mark.flaky for now.

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

@hannahbaumann hannahbaumann hannahbaumann requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@IAlibay IAlibay

@hannahbaumann hannahbaumann

Labels
None yet
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.

Add support for masked frames (i.e. positions missing at certain frames)
2 participants
@IAlibay @hannahbaumann