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

[BUG] QPD distribution and CyclicBoosting API non-compliance #190

Closed
fkiraly opened this issue Jan 29, 2024 · 4 comments · Fixed by #194
Closed

[BUG] QPD distribution and CyclicBoosting API non-compliance #190

fkiraly opened this issue Jan 29, 2024 · 4 comments · Fixed by #194
Labels
bug module:probability&simulation probability distributions and simulators module:regression probabilistic regression module

Comments

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 29, 2024

@setoguchi-naoki, @FelixWick, I have to apologize in advance for this.

Due to an error in the test logic introduced in an update to class retrieval (see #189), probability distributions went uncovered for most of your PR's duration.

This is my fault for not noticing, and breaking it in the first place.

Bad news is that the QPD distribution have a few non-compliances:

  • cdf method
  • mean, var
  • subsetting

CyclicBoosting as well:

  • _predict_quantiles arg should be alpha, not quantiles

Good news is that this has not been released yet, and I was running more tests before the 2.2.0 release, which is what ultimately caught the problem.

I'll simply wait with 2.2.0 until we had time to fix this - happy to help.

For testing locally, you need to depend on the branch #189 until it is merged.

@fkiraly fkiraly added bug module:probability&simulation probability distributions and simulators module:regression probabilistic regression module labels Jan 29, 2024
@fkiraly
Copy link
Collaborator Author

fkiraly commented Jan 30, 2024

Mechanically, to remove failures from main while reeabling tests:

(the tests should still be runnable via check_estimators)

@FelixWick
Copy link

@setoguchi-naoki Can you give this a try, please? I can help if you face any issues, of course.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Jan 30, 2024

FYI, I made a branch were all tests are activated - you can use this for debugging, or branch off it for a fix branch:
#194

I also attempted a naive fix to CyclicBoosting, renaming the argument: #195

@fkiraly
Copy link
Collaborator Author

fkiraly commented Jan 30, 2024

For CyclicBoosting, renaming args seems to have worked: #195

So we now only need to worry about the QPD distributions.

fkiraly added a commit that referenced this issue Apr 19, 2024
Re-enables tests for `QPD_U`, `QPD_S` skipped in #193 and
attempts to fix failures in QPD distributions.

it appears that #190 failures are resolved - fixes #190.

Further `QPD_B` seems to fail at mean and var calculations, this is a
separate bug and raised as #257.
As a mechanical consequence, `QPD_B` is left skipped in the tests.
fkiraly pushed a commit that referenced this issue May 14, 2024
…(cyclic-boosting ver.1.4.0) (#232)

Fixes #190 and #188

#### What does this implement/fix? Explain your changes.
- Modfied QPD's methods following interface of vectorized QPD
- Bug fix tests for QPD
- replace scipy.misc.derivative to findiff because it will be removed in
scypy 1.12.0

#### Does your contribution introduce a new dependency? If yes, which
one?

yes, I have extras dependency 
findiff: https://findiff.readthedocs.io/en/latest/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug module:probability&simulation probability distributions and simulators module:regression probabilistic regression module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants