Closed jamesmbaazam closed 5 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 99.07%. Comparing base (
a7faf3a
) to head (8bb0e64
). Report is 119 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@sbfnk Currently, if the data contains Inf
(more likely when an <epichains_summary>
is passed) and stat_max
is Inf
, it errors because R cannot generate an infinite sequence (See https://github.com/epiverse-trace/epichains/blob/5e6fd4966f1986eead1421f7298d50308768fa56/R/likelihood.R#L103-L104)
Would it make sense to calculate the likelihood using only the finite values and in the case where individual = TRUE
, return NA
where the entry is Inf
?
@sbfnk Currently, if the data contains
Inf
(more likely when an<epichains_summary>
is passed) andstat_max
isInf
, it errors because R cannot generate an infinite sequence (See) Would it make sense to calculate the likelihood using only the finite values and in the case where
individual = TRUE
, returnNA
where the entry isInf
?
I don't think it would be correct to exclude some of the outbreaks from the likelihood.
We can only have data containing Inf
with a finite stat_max
in the simulation, right? So it doesn't really make a huge amount of sense to then calculate the likelihood with stat_max
as Inf
. Perhaps this should be added to the summary as attribute anyway? The likelihood function could then read it out and set stat_max
to the "correct" value.
@sbfnk Currently, if the data contains
Inf
(more likely when an<epichains_summary>
is passed) andstat_max
isInf
, it errors because R cannot generate an infinite sequence (See https://github.com/epiverse-trace/epichains/blob/5e6fd4966f1986eead1421f7298d50308768fa56/R/likelihood.R#L103-L104) Would it make sense to calculate the likelihood using only the finite values and in the case where
individual = TRUE
, returnNA
where the entry isInf
?I don't think it would be correct to exclude some of the outbreaks from the likelihood.
We can only have data containing
Inf
with a finitestat_max
in the simulation, right? So it doesn't really make a huge amount of sense to then calculate the likelihood withstat_max
asInf
.Perhaps this should be added to the summary as attribute anyway?
This is already the case.
The likelihood function could then read it out and set
stat_max
to the "correct" value.
That makes sense. See change here 8a35bc7. Did you mean that?
This PR closes #39.
Please check if the PR fulfills these requirements
[x] I have read the CONTRIBUTING guidelines
[ ] A new item has been added to
NEWS.md
[x] Tests for the changes have been added (for bug fixes / features)
[x] Docs have been added / updated (for bug fixes / features)
[x] Checks have been run locally and pass
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
A feature
What is the current behavior? (You can also link to an open issue here)
likelihood()
works only with a numeric vector of chains.An
<epichains_tree>
or<epichains_summary>
object can be passed directly tolikelihood()
to estimate the likelihood of observing the chains.Not applicable.
~- Do we want to override the other relevant arguments (offspring_dist, stat_max, etc) with the attributes of an
<epichains_tree>
when passed, or should the user still supply them?~Inf
andstat_max
isInf
, it errors because R cannot generate an infinite sequence (See https://github.com/epiverse-trace/epichains/blob/5e6fd4966f1986eead1421f7298d50308768fa56/R/likelihood.R#L104).@sbfnk, how can we surmount this issue?
Would it make sense to calculate the likelihood using only the finite values and in the case where
individual = TRUE
, returnNA
for theInf
values?