Fix max-age clade splitting in as_daisie_datatable()#61
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses #60.
A
as_daisie_datatable()bug mishandled island colonists with branching times older thanisland_agein two ways:while (brts[1] >= island_age)) never re-evaluated, so it relied on the loop body emptyingevent_timesto break. Every event inevent_times— including the colonisation time and valid in-island branching times — was emitted as its own*_MaxAgesingleton row.event_times[1]was being written into the first split row, producing aBranching_timesvalue older thanisland_agefor that singleton (the symptom reported in Bug inas_daisie_datatable()when branching times older than island age #60).Changes
For an island colonist where one or more branching times exceed
island_age:*_MaxAgesingleton row, with the clade name suffixed_1,_2, ...;island_age(i.e. valid in-island cladogenesis), stays as the main*_MaxAgerow under the original clade name.If every branching time exceeds
island_age, the main row contains just the colonisation time. Numeric clamping toisland_age - epsscontinues to happen increate_daisie_data(), as before.event_times[2]so the colonisation time at index 1 is preserved.event_times[2] >= island_age), guarded bylength(event_times) >= 2so the loop exits cleanly once only thecol_timeremains.*_MaxAgewhen its largest event still exceedsisland_age.@detailssection documenting the_MaxAgesplitting semantics.tests/testthat/test-as_daisie_datatable.R: updated three existing test expectations to reflect the corrected splitting; and added a regression test using the exact reproducer from Bug inas_daisie_datatable()when branching times older than island age #60.NEWS.md: bug-fix entry under the development version section.