-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fixing issues with inference and NAs #292
Conversation
Running tests to ensure this is working |
Summary of changes:
|
Added an quick fix change to change the default of the total aggregation likelihood to A later enhancement would be to add these sorts of options to the config - eg aggregation dates (in the emcee version, there is a python W-SAT option), and any summed likelihoods considered, or other modifications... |
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.
Very important bug fixes
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.
We will need to come back and add unit tests to demonstrate that this change did in fact fix the underlying issue so don't close the corresponding issue yet.
Yeap tests for inference are 100% required. would be great. Thanks both for
review
…On Fri, 13 Sept 2024, 10:39 am Timothy Willard, ***@***.***> wrote:
***@***.**** approved this pull request.
We will need to come back and add unit tests to demonstrate that this
change did in fact fix the underlying issue so don't close the
corresponding issue yet.
—
Reply to this email directly, view it on GitHub
<#292 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKZGJPXMT4ZWMVJUKCNCEGDZWMBM5AVCNFSM6AAAAABMG62472VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMBTGQ3DGMJSGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
agree on the test @TimothyWillard, but thanks Sara for finding and fixing this |
Fixing issues with inference and NAs
Describe your changes.
What does your pull request address? Tag relevant issues.
#286 and related prior issue #272 (do need to filter dates to be the same but was just in the wrong spot)
Mentions of relevant team members.
@shauntruelove @alsnhll