shirtsgroup / physical_validation

Physical validation of molecular simulations
https://physical-validation.readthedocs.io
MIT License
55 stars 19 forks source link

Follow-up from #86 - rename bs_seed and think about additional tests #100

Closed ptmerz closed 3 years ago

ptmerz commented 3 years ago

In #86, @mattwthompson made some good comments. We should

LGTM, to the extent that I can check code by eye. Might be worth tackling #89 first, then add a test to ensure that the seed is actually propagaged through to results? i.e. run twice with the same seed and then twice with the different seed, compare results

This was added as a prerequisite of #89, because the tests introduced in #89 won't pass without having the bootstrap seed properly used. Introducing #89 first would require to change the test to ignore the output affected by the bootstrapping (such that it still passes CI), and then change them back when introducing this. In hindsight, this might have been a better way to go about it, but it seems like overkill now. Testing locally, I can confirm, however, that #89 fails without the current change.

We could still add additional tests as a follow-up to #89. We'll have to be careful in deciding what we want to test using end-to-end tests, however - they are relatively slow, so anything that can be unit-tested rather than regression tested would be better.

As a matter of taste, I may suggest a more descriptive variable name like bootstrap_seed

I'd agree. Maybe a follow-up?

_Originally posted by @ptmerz in https://github.com/shirtsgroup/physical_validation/issues/86#issuecomment-769624996_

ptmerz commented 3 years ago
  • [ ] Rename bs_seed to something more descriptive

Consider doing the same for the bootstrapping related quantities in ensemble.py (to avoid merge conflicts, wait until #85 was merged - or simply work off that branch).