-
Notifications
You must be signed in to change notification settings - Fork 0
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
Rewrite ExpSurvOS to prevent NaNs #73
Conversation
Code Coverage Summary
Diff against main
Results for commit: 0c56de3 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Hey @holgstr - Please can you add a unit test for this change. E.g. have some code that would have caused the bug / error on the old code which now works fine on this new code ? Tests code goes in the |
Hey @gowerc, thanks for the quick look, I have added the test now. |
@holgstr - Apologies I'm not overly familiar with this project, which equation / paper are you referencing with this change ? |
@gowerc It's S_OS(t) on page 4 of Erdmann et al (2023). |
I didn't have time to do the algebra to be confident that, outside of the mentioned edge case, the before and after are identical e.g. that
Is equivalent to
EDIT - Got around to doing the algebra to confirm they are indeed identical :) Left code in for reference though But running a load of different numbers I can't find any examples where its not true and all the prior unit tests still pass so I'm happy for this to be approved. Code for reference:
@holgstr - Not essential but may be worth just adding a note to the NEWS.md file listing the bug fix as a not-yet-released change. See here for an example of an "in-development" update where the # number is the number of the PR (73 in this case) That being said I just realised I don't have permissions to approve the PR 😢 @numbersman77 / @niessl - Would 1 of you be able to approve please; or alternatively could you give me permissions to approve please. |
NEWS.md
Outdated
|
||
## Minor improvements and bug fixes | ||
|
||
- ExpSurvOS now returns 0 instead of NaN for large values of t (#73, @holgstr) |
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.
Apologies I wasn't clear, this should go at the top of the file as this will eventually be included into a next version see:
@gowerc was this what you meant? Bc now there is a version conflict between Description and News? :) Maybe I should substitute "(development version)" with "0.0.5.9011" ? |
@cicdguy - It looks like the Version check pipeline is designed to fail if the DESCRIPTION version number doesn't exactly match the version number in the NEWS.md file. Can I double check what the expected/standard workflow is here then. I usually just add everything as |
Yeah that workflow only checks 2 things:
The former is used in conjunction with the version bump workflow. I'd recommend not using it if you prefer to use |
As this isn't my package I won't advocate for disabling / changing any of the pipelines. |
@holgstr - Approved feel free to merge |
@gowerc Great, thanks a lot! |
Fixes #72