Skip to content

Add archive to results script - #130

Open
jthorton wants to merge 4 commits into
mainfrom
jacs_run_ashgc
Open

Add archive to results script#130
jthorton wants to merge 4 commits into
mainfrom
jacs_run_ashgc

Conversation

@jthorton

Copy link
Copy Markdown
Contributor

Fixes #129 using the protocol to estimate the result and the uncertainty.

Notes:

  • the script processes one archive at a time and creates the corresponding computational_results.json file which will then need to be combined to create a single submission
  • it assumes the inputs were generated using the provided example scripts
  • it requires there to be at least 3 repeats by default as we assume benchmarks are always done in triplicate
  • it supports ASFE and Hybrid topology RBFE currently but can be expanded to other protocols

@jaclark5 jaclark5 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks so clean compared to the other version! I like the use of protocols. I have a couple small changes.

There are two aspects of the script from the other PR that we aren't addressing here, and another that arises. I'm not opposed to merging this but we do need to work out:

  1. Does it make sense to separate analysis of alchemical archives from network jsons or pulling directly from alchemiscale network keys/org;project;scope? That will be duplicate code (unless I'm missing something about what protocols do/don't work on).
  2. Do you want to support submissions with no repeats that uses mbar uncertainty?
    We were adding that capability for me and my tyk2 submission (#127 )... but maybe that's not a good enough reason? If we don't want anyone to do that in production then we shouldn't merge my other PR.
  3. In production, will we want the ability to generate a computational_results.json structure from multiple alchemical archives?, Is there a different answer to wanting to generate a computational_result.json from multiple network.json files?
    Considerations:
    • If the answer to (2) is no, I could run another jacs_set/tyk2 network with two repeats and #127 can include that as a test case for multiple alchemical archives. However, it could make more sense to go into alchemiscale and action a couple more repeats and regenerate the AlchemicalArchive as a single result... but is such an extension that easy outside of alchemiscale? Would non-alchemiscale calculations require multiple AlchemicalArchives to append repeats?
    • What if in the future we want to increase our minimum to 5 and we want to run 2 more repeats after the alchemiscale stack has been updated? Should we not consider those cases and just expect that we will rerun all 5?

I think we need to answer these questions before merging this and #127. @jthorton @IAlibay @hannahbaumann. I can do an early morning meeting next week if we want to discuss.

Comment thread openfe_benchmarks/scripts/generate_results_archives.py Outdated
Comment thread openfe_benchmarks/scripts/generate_results_archives.py Outdated
Comment thread openfe_benchmarks/scripts/generate_results_archives.py Outdated
Comment thread openfe_benchmarks/scripts/generate_results_archives.py
Comment thread openfe_benchmarks/scripts/generate_results_archives.py
jthorton and others added 3 commits July 27, 2026 11:28
@jthorton

jthorton commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @jaclark5

Do you mean we should combine pulling the archive from alchemiscale in this script with this results generation? If so I think that could be a good idea to reduce the number of scripts to run and the other script is quite simple.

If you mean the local results analysis using networks and raw JSON results it might be a good idea to keep them separate for now until quickrun supports making archives locally then we can use the same code for both result types.

My take from our recent meeting was that we would ensure benchmarks are always done in at least triplicate to take any issues with results interpretation (mbar vs std) out of the equation.

Happy to add support for many archives to this script but we will need to workout how to handle the cases where edges might overlap. This could happen if two different sets of charges/force fields were used for the same edges, we would have repeated values for the DGs with no way of tracking which DDGs contributed to them. I think we either need to do some error handling to stop this and tell the user o make two submissions or assume that users know what they are doing and just report the results.

Considerations

I agree that just actioning more tasks is the right way to go, and that submitting a single archive for that network is the right approach. I think we need to make it clear in the guide that we expect each archive to contain all repeats of the alchemical network.

Oh thats a good question maybe the others have more thoughts on this but, we only list a single openfe version in the submission so we would probably want to run them again using the new version to keep them consistent rather than mixing the results?

@jaclark5 jaclark5 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok it seems like from all the answers to my questions:

  • this will remain a separate script from local analysis
  • all archives must have multiple repeats (no optional use of mbar uncertainty)
  • no support for multiple archives, one archive per submission.

With that finalized, it looks good to go!

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.

Use the protocol result to get edge estimates

2 participants