Skip to content
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

All records get a different draw from EPS #1110

Merged
merged 2 commits into from
Aug 23, 2023
Merged

All records get a different draw from EPS #1110

merged 2 commits into from
Aug 23, 2023

Conversation

kylebaron
Copy link
Collaborator

@kylebaron kylebaron commented Aug 22, 2023

Previous behavior was to assign a new EPS only when time was advancing in the data set within individual. Now, we assign a new EPS every record.

There was some linting that happened too ... not sure if this came from Rstudio or not. But let's leave it as is.

@kylebaron kylebaron marked this pull request as ready for review August 23, 2023 02:21
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The code change makes sense and looks straightforward (though I don't have a solid grasp of all the moving parts).

  • I confirmed the new test fails if I revert the code change.

  • I confirmed the internal cases that prompted this PR are resolved by this change (i.e. we no longer need to force any covariance matrices to be positive definite when calculating npde).

  • Reading through the internal discussion, it sounds like there is consensus about this change in behavior.

  • As far as I understand, this can be viewed as a pure bug fix. In that case, I think unconditionally changing the behavior along with a NEWS item is sufficient, as opposed to doing something more involved to retain the current behavior.

Copy link

@timwaterhouse timwaterhouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Agree with @kyleam that the change along with the NEWS item is sufficient.

@kylebaron kylebaron merged commit e41580e into main Aug 23, 2023
@kylebaron kylebaron mentioned this pull request Aug 24, 2023
@kylebaron kylebaron deleted the fix/eps branch December 14, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants