Add archive to results script - #130
Conversation
jaclark5
left a comment
There was a problem hiding this comment.
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:
- 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).
- 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. - 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.
Co-authored-by: Jennifer A Clark <jennifer.clark@omsf.io>
Co-authored-by: Jennifer A Clark <jennifer.clark@omsf.io>
|
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
left a comment
There was a problem hiding this comment.
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!
Fixes #129 using the protocol to estimate the result and the uncertainty.
Notes: