choderalab / perses

Experiments with expanded ensembles to explore chemical space
http://perses.readthedocs.io
MIT License
178 stars 50 forks source link

Evaluating unit test coverage; filling in coverage where needed #1177

Closed dotsdl closed 1 year ago

dotsdl commented 1 year ago

Evaluating coverage of unit tests for NonEquilibriumCyclingProtocol, adding tests where we still have gaps.

ijpulidos commented 1 year ago

I see the CI tests are failing because the environment is not using the latest openff-toolkit and openff-units.

ijpulidos commented 1 year ago

Ah actually is the lack of openff-models, rather. I'll fix that.

codecov[bot] commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (protocol-neqcyc@7351d9c). Click here to learn what that means. The diff coverage is n/a.

ijpulidos commented 1 year ago

I guess retrying didn't really help https://github.com/choderalab/perses/actions/runs/4736629137/jobs/8408413871?pr=1177#step:9:2086

dotsdl commented 1 year ago

@ijpulidos how's this looking for unit handling on get_estimate and get_uncertainty?

dotsdl commented 1 year ago

I updated the default settings to match recommendations for production settings from @jchodera. This will avoid users submitting very short runs by default; I'm doing live of testing these choices now. Aiming to merge this PR in the morning.

@ijpulidos if you get a chance to look over my choices in the default settings, please do! Just trying to get things reasonable and you have more intuition for this than I do!

ijpulidos commented 1 year ago

@dotsdl They seem good! Thanks for changing these.

dotsdl commented 1 year ago

@ijpulidos I think we're good to go here. Ran a burn test on my local box to check for memory leaks (host and GPU), and didn't observe behavior that strongly suggested any.

Can you give this a thumbs up then I'll merge?

dotsdl commented 1 year ago

Got one more fix to put in.

dotsdl commented 1 year ago

Nvm, all good I think.

ijpulidos commented 1 year ago

@dotsdl This looks good to me, great work! Feel free to merge