sktime / skpro

A unified framework for tabular probabilistic regression and probability distributions in python
https://skpro.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
231 stars 45 forks source link

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

Closed fkiraly closed 4 months ago

fkiraly commented 7 months ago

@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 https://github.com/sktime/skpro/pull/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:

CyclicBoosting as well:

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 commented 7 months ago

Mechanically, to remove failures from main while reeabling tests:

(the tests should still be runnable via check_estimators)

FelixWick commented 7 months ago

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

fkiraly commented 7 months ago

FYI, I made a branch were all tests are activated - you can use this for debugging, or branch off it for a fix branch: https://github.com/sktime/skpro/pull/194

I also attempted a naive fix to CyclicBoosting, renaming the argument: https://github.com/sktime/skpro/pull/195

fkiraly commented 7 months ago

For CyclicBoosting, renaming args seems to have worked: https://github.com/sktime/skpro/pull/195

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