-
Notifications
You must be signed in to change notification settings - Fork 6
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
Resolve symmetric migrations into pairs of asymmetric migrations. #268
Conversation
Codecov Report
@@ Coverage Diff @@
## main #268 +/- ##
==========================================
+ Coverage 96.14% 96.69% +0.54%
==========================================
Files 7 7
Lines 1064 1058 -6
==========================================
Hits 1023 1023
+ Misses 41 35 -6
Continue to review full report at Codecov.
|
Also restores the migration simplification code that was removed in commit e2a7e05 (popsim-consortium#209). Closes popsim-consortium#263.
Fixes jupyter-book/docutils-0.17 incompatilibity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me! Lots of fiddling, but I think it will end up simplifying things again quite a bit. I also like that it gets the migration simplification back in there when using asdict_simplified
:)
@@ -11,7 +12,6 @@ | |||
Builder, | |||
Epoch, | |||
AsymmetricMigration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only have AsymmetricMigration as the internal continuous migration class, should we call it simply Migration? Or leave it as AsymmetricMigration so that we leave open the option of reintroducing SymmetricMigration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good question. I left it as AsymmetricMigration
initially because it was easiest at the first stage of refactoring. But having this be explicit might help reduce confusion (at the expense of verbosity). And as you say, its not implausible that we'd introduce something called SymmetricMigration again in the future. But I don't have a strong opinion on what this should be called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, it was more of a question than suggestion that we should change it anyway. I'm happy to keep it as AsymmetricMigration, as it does reduce any possible confusion (and also allows us to introduce a SymmetricMigration if we are compelled to in the future again). Most users will still just see the add_migration
function in the Builder, and won't even know how it is being represented behind the scene, I imagine.
Also restores the migration simplification code that was removed
in commit e2a7e05 (#209).
Closes #263.