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

Add gather support for parallel dir structure #1044

Merged
merged 20 commits into from
Dec 20, 2024

Conversation

atravitz
Copy link
Contributor

@atravitz atravitz commented Dec 9, 2024

Checklist

  • Added a news entry

Developers certificate of origin

Copy link

github-actions bot commented Dec 9, 2024

🚨 API breaking changes detected! 🚨

Copy link

github-actions bot commented Dec 9, 2024

🚨 API breaking changes detected! 🚨

@atravitz atravitz linked an issue Dec 9, 2024 that may be closed by this pull request
Copy link

🚨 API breaking changes detected! 🚨

Copy link

🚨 API breaking changes detected! 🚨

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Couple comments on the protocol side of things, not read the rest yet.

openfe/protocols/openmm_rfe/equil_rfe_methods.py Outdated Show resolved Hide resolved
openfe/protocols/openmm_rfe/equil_rfe_methods.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.73%. Comparing base (76127a1) to head (f4e753a).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
openfecli/commands/gather.py 77.27% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
- Coverage   94.57%   92.73%   -1.84%     
==========================================
  Files         135      135              
  Lines       10061    10077      +16     
==========================================
- Hits         9515     9345     -170     
- Misses        546      732     +186     
Flag Coverage Δ
fast-tests 92.73% <90.00%> (?)
slow-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link

🚨 API breaking changes detected! 🚨

Copy link

🚨 API breaking changes detected! 🚨

@atravitz atravitz requested review from mikemhenry and removed request for mikemhenry December 12, 2024 16:16
@atravitz atravitz changed the title [WIP] Add gather support for parallel dir structure Add gather support for parallel dir structure Dec 12, 2024
@atravitz atravitz marked this pull request as ready for review December 12, 2024 16:46
@atravitz atravitz requested a review from mikemhenry December 12, 2024 16:46
Copy link

🚨 API breaking changes detected! 🚨

Copy link

🚨 API breaking changes detected! 🚨

Copy link

🚨 API breaking changes detected! 🚨

@atravitz atravitz mentioned this pull request Dec 13, 2024
2 tasks
@atravitz atravitz requested review from hannahbaumann and jthorton and removed request for mikemhenry December 13, 2024 23:38
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 @atravitz , this looks great. Only thing I'm not so sure about yet is the input file path for gather.
I wanted to test this on one of my old result files, however they were in a structure such as this:
all_results with following subfolders:
results_0, results_1, results_2, results_other_setting_0, results_other_setting_1, results_other_setting_2.
If I only want to analyze the results from the first three folders, I'd have to move the other folders in a different directory.
This could be fine, but alternatively, gather could also take a list of Paths. I'm not sure yet what is better, but just wanted to raise this use case.

openfe/protocols/openmm_rfe/equil_rfe_methods.py Outdated Show resolved Hide resolved
@hannahbaumann hannahbaumann self-assigned this Dec 17, 2024
@IAlibay
Copy link
Member

IAlibay commented Dec 17, 2024

gather could also take a list of Paths.

Just wanted to chime in here that having gather take in a list of results JSON and/or directories would be great (the former more than the latter)! That being said I do think this is maybe something to be dealt with elsewhere.

@atravitz
Copy link
Contributor Author

I agree with both of you @IAlibay @hannahbaumann - how do you feel about me addressing this in a follow-up PR?

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 @atravitz , lgtm!

Copy link

No API break detected ✅

@atravitz atravitz merged commit 66476fd into main Dec 20, 2024
11 checks passed
@atravitz atravitz deleted the add_gather_support_parallel_dir_structure branch December 20, 2024 17:05
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.

gather should support simulation repeats submitted in parallel
4 participants