[ENH] Cyclic boosting interface

[setoguchi-naoki]

Reference Issues/PRs

#132

What does this implement/fix? Explain your changes.

① The interface for Cyclic boosting
J-qpd estimation by quantile regression with Cyclic boosting, Output distribution's parameters are given by j-qpd's loc and scale

② New distribution class, Johnson quantile parameterized-distribution

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

cyclic_boosting package>=1.2.1

What should a reviewer concentrate their feedback on?

Please check skpro.distributions.qpd especially, I'd like to get the feedback J_QPD_S' method behaivier mutch or not to skpro's manner

Did you add any tests for the change?

No

Any other comments?

PR checklist

For all contributions

• I've added myself to the list of contributors with any new badges I've earned :-)
  How to: add yourself to the all-contributors file in the skpro root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
  See here for full badge reference
• The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

For new estimators

• I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
• I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
• If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
  dependency isolation, see the estimator dependencies guide.

[fkiraly]

looks good!

Did this get closed by accident?

[setoguchi-naoki]

@fkiraly
Hi, Franz, I'm one of the contributor of Cyclic boosting. I closed the PR manually so that I wanted to get review from another contributor of Cyclic boosting on my forked repository because my code is a draft.

[fkiraly]

I closed the PR manually so that I wanted to get review from another contributor of Cyclic boosting on my forked repository because my code is a draft.

My opinion, it's best to have reviews here - general best practice recommendation to have all discussion in the "host repo" and not in the fork, as the host repo is indexed by various tools, while fork repos are in general not (and can also disappear more easily).

It's marked as draft too, so no danger of getting it merged.

Also feel free to ask any of the skpro contribs for review!

[setoguchi-naoki]

@fkiraly
Thank you for giving me your opnion and follow your comment. I have finished to implement for interface of Cyclic boosting, so Would you review the code for me? I concern weather my implementation fit skpro's manner or not.

[setoguchi-naoki]

@fkiraly
As Felix said, feature_properties (and feature_groups) always don't need to set up. So I just updated CyclicBoosting class's paramaters and removed specific test paramaters as a most simple solution. if feature_properties and feature_groups are None, these are set automatically in cyclic boosting model. I guess it should passed all test by this modification.

[fkiraly]

Excellent, thanks!

Could you perhaps add a test that uses the manual feature typing to test_cyclic_boosting, so we have that covered?

Otherwise, I have no additional requests, very nice.

Once you think you are ready, pleas switch this PR to "ready for review".

[setoguchi-naoki]

I have finished :)

[fkiraly]

Nice!
All green for merge if tests pass.

[fkiraly]

Fails with test_cyclic_boosting_with_manual_paramaters - ValueError: The SPT values need to be monotonically increasing. - error seems genuine.

[FelixWick]

The quantile predictions need to be ordered correctly for the QPD to make sense. And as we are dealing with statistical models, it can happen that this is not the case, especially in low-statistics tests. So, I would suggest to just tune the respective test ;).

[fkiraly]

that, or adding a coercion?

[FelixWick]

Yeah, I have implemented a potential coercion in QPD_RegressorChain in Cyclic Boosting. But a simpler approach would do as well here, I think.

[setoguchi-naoki]

I modified SPT's value checking from replacement by mean to sorting.

[fkiraly]

Seems lik there are sporadic timeouts of test_cyclic_boosting_with_manual_paramaters - a single test run is limited to 10min or it will fail with the assumption that the long runtime is a consequence of a failure.

[setoguchi-naoki]

All test have passed :)

[fkiraly]

🎉 🎆 🎉 🎆 🎉

[fkiraly]

Minor question, non-blocking, should the QPD not go in the "non-parametric" section of the distributions?

They are called "quantile parameterized" but afaik are typically considered non-parametric since they are explicit in terms of quantiles (which are interpolated) rather than intrinsic simple parameters.

[fkiraly]

Just looking through open convos at the moment, and I found this by @FelixWick:

But as I wrote below, I think we should rename the parameter alpha in predict_quantiles, because it is confusing.

Apologies, I overlooked this - are you suggesting that the interface be changed in general for all estimators, with a rename of alpha? There might be a case for that, though I cannot find the arguments you are referencing.

[FelixWick]

Minor question, non-blocking, should the QPD not go in the "non-parametric" section of the distributions?

They are called "quantile parameterized" but afaik are typically considered non-parametric since they are explicit in terms of quantiles (which are interpolated) rather than intrinsic simple parameters.

Yes, that sounds reasonable.

[FelixWick]

Just looking through open convos at the moment, and I found this by @FelixWick:

But as I wrote below, I think we should rename the parameter alpha in predict_quantiles, because it is confusing.

Apologies, I overlooked this - are you suggesting that the interface be changed in general for all estimators, with a rename of alpha? There might be a case for that, though I cannot find the arguments you are referencing.

No, that was not a general comment, just for Naoki's implementation. But I think he fixed it. Right, @setoguchi-naoki ?

[fkiraly]

Minor question, non-blocking, should the QPD not go in the "non-parametric" section of the distributions?

Yes, that sounds reasonable.

Ok, moving to the non-parametric taxon then.

[setoguchi-naoki]

Just looking through open convos at the moment, and I found this by @FelixWick:

But as I wrote below, I think we should rename the parameter alpha in predict_quantiles, because it is confusing.

Apologies, I overlooked this - are you suggesting that the interface be changed in general for all estimators, with a rename of alpha? There might be a case for that, though I cannot find the arguments you are referencing.

No, that was not a general comment, just for Naoki's implementation. But I think he fixed it. Right, @setoguchi-naoki ?

Yes, it is only for my predict_quantiles implementation now.