Changes to Systematics manager to support MABE #451
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #451 +/- ##
==========================================
+ Coverage 77.56% 77.92% +0.35%
==========================================
Files 334 337 +3
Lines 39631 40450 +819
==========================================
+ Hits 30740 31519 +779
- Misses 8891 8931 +40 ☔ View full report in Codecov by Sentry. |
|
I'm going to close this PR since this branch is merged into MABE_devel, which will presumably eventually get merged into main (it would be useful to do that sometime in the near future) |
|
Actually, I take that back - just noticed that checks aren't running on the main MABE_devel PR and it would be useful to be able to see them, so I'm going to re-open this for now (but the plan should be to merge MABE_devel, not this one) |
|
@mmore500 I thought this was all going to get merged in with master during the mega merge, but it looks like it didn't. Any chance I could get a review? The systematics on master is way out of date and buggy |
|
On it! |
|
Thanks so much! |
|
(sorry, found one more tiny change hiding in my local phylotrackpy repo) |
include/emp/Evolve/Systematics.hpp
Outdated
| fun_calc_info_t calc_info_fun; ///< Function that takes an organism and returns the unit being tracked by systematics | ||
| Ptr<taxon_t> next_parent; ///< The taxon that has been marked as parent for next new org | ||
| Ptr<taxon_t> most_recent; ///< The most-recently added taxon | ||
| bool num_orgs_wrong = false; ///< Keep track of whether we have loaded from a file that didn't |
There was a problem hiding this comment.
Could use std::optional for num_orgs and for total_offspring instead
There was a problem hiding this comment.
Hmm, I'm conflicted. I like the safety/clarity that optional would provide. However, it looks like it's a not insubstantial time/space hit for numbers that are used in every taxon and operated on a fair amount. It will also make the code potentially confusing to folks who are not familiar with std::optional. Particularly because it's actually very expedient to assume that trees loaded in without org counts have one of each org. It makes all of the topology stuff and tree modification operations work fine. It just makes a couple of metrics inaccurate.
There was a problem hiding this comment.
Ah, so when num_orgs_wrong is true, num_orgs is still being used but just at a defaulted value? Maybe name these variables _defaulted instead of _wrong?
There was a problem hiding this comment.
It should compile down to the same thing https://godbolt.org/z/M8j9fG7js To save space compared to std::optional you'd have to do something like pack the two bools into a flag field instead of having two separate bools.
There was a problem hiding this comment.
I'm happy to switch to "defaulted" (although that kind of feels like a euphemism for wrong 😅) if you think that would be clearer.
I could be wrong here, but isn't part of the reason it's compiling down to the same thing on godbolt the fact that in that case the compiler knows everything ahead of time and so can optimize out branching introduced by std::optional?
There was a problem hiding this comment.
That''s what I was guessing (which I think actually would be concerning, because if we're using optionals we're going to have be passing them around, etc. - it's semantically different, but representative of the scenarios we're comparing), but then I tested it out and it looks like it is really just the deref that makes the difference
There was a problem hiding this comment.
Okay, that's fair for _defaulted.
There was a problem hiding this comment.
Either way could be a good call!
There was a problem hiding this comment.
I was almost convinced, but here's the problem. The dereference operator is undefined behavior if there's not actually a value in the optional object. So then you end up needing to use .value(). Which A) appears to be back to the slow speed (https://quick-bench.com/q/acv1zCs2Agso3yZKhfBolRQ8rx0) and B) potentially throws an exception, which we would love to avoid ever having happen in code compiled via emscripten.
There was a problem hiding this comment.
I'm going to go ahead and just used _defaulted, because the cases where we care that its sometimes wrong are way rarer than the cases where we don't. If that balance shifts we can reconsider.
Co-authored-by: Matthew Andres Moreno <[email protected]>
Co-authored-by: Matthew Andres Moreno <[email protected]>
… mabe-systematics
Co-authored-by: Matthew Andres Moreno <[email protected]>
… mabe-systematics
Co-authored-by: Matthew Andres Moreno <[email protected]>
This pull request will eventually contain everything necessary on the Empirical side to make systematics tracking in MABE work. Depends on #419.