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

359: Check PIT #916

Merged
merged 9 commits into from
Sep 21, 2024
Merged

359: Check PIT #916

merged 9 commits into from
Sep 21, 2024

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Sep 19, 2024

Description

This PR closes #359.

Most things look correct to me - I have addressed the issue of num_bins not being reflected by ensuring 0 and 1 are always included as boundaries and cleaned up some of the other code/docs (see individual commit messages).

There were two broader questions raised by this work:

  • the way get_pit and plot_pit interact makes addressing Randomised -> non-randomised PIT for count values #913 impossible - the nonrandomised method requires the bins to be known at the time the PIT is calculated
  • the way quantiles are plotted when the breaks don't coincide with the quantiles levels in the data doesn't really makes sense (only the ones that coincide are plotted). It might be nice here to make a (e.g. linear, optional) interpolation of the CDF, in which case we could plot a PIT with arbitrary breaks

Both of these cases could be solved if the get_pit() method is changed to take the bins/breaks as an argument, all calculations/approximations are moved there, and plot_pit just plots the resulting geom_col (which may or may not even be needed as a separate function - e.g. get_pit could have a plot argument or else it could just be left to the user). I would suggest to make this change (in a separate issue/PR combo).

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title as follows: issue-number: PR title
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • I have built the package locally and run rebuilt docs using roxygen2.
  • My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

Copy link
Contributor

@nikosbosse nikosbosse 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, thanks a lot!
I'm all in favour of your suggestion to move the heavy lifting to get_pit

@nikosbosse nikosbosse merged commit 9d28b4b into main Sep 21, 2024
8 of 9 checks passed
@nikosbosse nikosbosse deleted the check-pit branch September 21, 2024 10:05
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.

Testing: Make sure pit() and plot_pit() are correct
2 participants