Skip to content

use posterior::gpdfit and posterior::qgeneralized_pareto()#305

Open
avehtari wants to merge 7 commits intomasterfrom
use-posterior-gpdfit
Open

use posterior::gpdfit and posterior::qgeneralized_pareto()#305
avehtari wants to merge 7 commits intomasterfrom
use-posterior-gpdfit

Conversation

@avehtari
Copy link
Copy Markdown
Member

Since we do use posterior package elsewhere,
replace gpdfit() and qgpd() with posterior::gpdfit and posterior::qgeneralized_pareto()

This will make maintenance easier. For example, there is a PR for posterior::gpdfit() to make it more robust and posterior::qgeneralized_pareto() already is more robust than qgpd()

This is related to #249, but timing is due to the fix PR for posterior::gpdfit()

@avehtari avehtari requested a review from jgabry October 17, 2025 17:10
Copy link
Copy Markdown
Member

@jgabry jgabry left a comment

Choose a reason for hiding this comment

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

I think the R cmd check error is because the NAMESPACE file is still trying to export gpdfit from loo. If you run devtools::document() it should fix this and then we can see if all the tests pass.

@avehtari
Copy link
Copy Markdown
Member Author

If it has been exported, then someone might be using it? Should I put it back and define it with posterior::qgeneralized_pareto()?

@avehtari
Copy link
Copy Markdown
Member Author

At least in github only I and Juho Timonen have used loo::gpdfit, so I guess it can be removed

@jgabry
Copy link
Copy Markdown
Member

jgabry commented Oct 17, 2025

Good point. Yeah you can just replace the bodies of the functions with calls to the posterior versions.

@jgabry
Copy link
Copy Markdown
Member

jgabry commented Oct 17, 2025

At least in github only I and Juho Timonen have used loo::gpdfit, so I guess it can be removed

OK in that case, we can try removing it and we will find out in the reverse dependency checks whether any packages are using it

@jgabry
Copy link
Copy Markdown
Member

jgabry commented Oct 17, 2025

The error now is because apparently these functions aren't exported from posterior, they're just internal functions. That would be easy to change but it would require doing a CRAN release of posterior.

@avehtari
Copy link
Copy Markdown
Member Author

my bad, I guess this needs to wait for the next posterior release

@jgabry
Copy link
Copy Markdown
Member

jgabry commented Oct 17, 2025

There is also #290 from @VisruthSK that is waiting to be merged until there is a new release of posterior

@VisruthSK VisruthSK marked this pull request as draft February 21, 2026 21:30
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.71%. Comparing base (23f2117) to head (dce7e3c).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   92.78%   92.71%   -0.07%     
==========================================
  Files          31       30       -1     
  Lines        2992     2965      -27     
==========================================
- Hits         2776     2749      -27     
  Misses        216      216              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@VisruthSK VisruthSK marked this pull request as ready for review April 7, 2026 17:40
@VisruthSK VisruthSK requested a review from jgabry April 7, 2026 17:41
@jgabry
Copy link
Copy Markdown
Member

jgabry commented Apr 7, 2026

I think this needs to bump the posterior version in DESCRIPTION (unless it's merged after the other one that does that, but not sure which will be merged first)

@VisruthSK
Copy link
Copy Markdown
Member

Right, in my head it was being merged after the previous one. Will fix that now.

@jgabry
Copy link
Copy Markdown
Member

jgabry commented Apr 7, 2026

Right, in my head it was being merged after the previous one. Will fix that now.

We need to figure out what to do about the warnings in the other PR, so will probably do this one first. But for this one, it occurs to me that we need to reexport gpdfit from posterior in order to avoid breaking backwards compatibility (it's currently exported from loo so could be called as loo::gpfit in someone's code.

@VisruthSK
Copy link
Copy Markdown
Member

Will restore tests too then

@jgabry
Copy link
Copy Markdown
Member

jgabry commented Apr 7, 2026

Do you think we need to test it in addition to exporting it? I assume it's tested in posterior. Maybe good to test here too, but I'm not sure

@VisruthSK
Copy link
Copy Markdown
Member

If we are rexporting it I think it makes to test, mainly so that the API is the same no? The tests were super minimal anyway.

@jgabry
Copy link
Copy Markdown
Member

jgabry commented Apr 7, 2026

Ok yeah good call

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.

4 participants