[MNT] Deprecation message for `CyclicBoosting` changes #320

setoguchi-naoki · 2024-05-12T15:45:55Z

Reference Issues/PRs

#232

What does this implement/fix? Explain your changes.

Add deprication warning
Docstring

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

No

What should a reviewer concentrate their feedback on?

I'm not familiar with the detail for deprecation process and which future version we should set for removing default parameters.

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 · 2024-05-12T21:17:16Z

skpro/distributions/qpd.py

+        comment = "Parameter 'lower' and 'upper' will be moved to \
+                   class parameters in version X.X.X. \
+                   To retain prior behaviour, set 'lower' to 0.0 \
+                   and 'upper' to large number explicitly"


should this not be None instead of a large number?

It should be a large number here because 'upper' is used as the range of the differential. However, I made a mistake in the docstring for the default value of 'upper ', it is not None but 1e3 in the future version. In this case too, the large number should be set to the 'upper' parameter to cover the range of the user's dataset.

fkiraly

I think this is good, given the changes.

I would improve it as follows:

the message in __init__ needs only be raised if the changing parameters are invoked with a non-default, since that is the only case where behaviour changes or can break.

The parameters in the methods imo can be removed without deprecation message, as they were not API compliant

setoguchi-naoki · 2024-05-14T09:19:44Z

the message in init needs only be raised if the changing parameters are invoked with a non-default, since that is the only case where behaviour changes or can break.

Thanks, I modified as following your comment. the default values of 'lower' and 'upper' will be removed in the future version. I made a wrong deprecation message. It is also modified. If the default value is removed, a deprecation message is always raised, as this can break the user's code.

The parameters in the methods imo can be removed without deprecation message, as they were not API compliant

Sorry, I'm not sure I understand your comment. Which method did you mention?

setoguchi-naoki · 2024-05-14T09:25:17Z

In addition, what future version should we specify in deprecation message?

fkiraly · 2024-05-15T22:36:44Z

2.4.0 - the second next MINOR release.

I've opened a new PR here that covers only the regressor class: #329
The distribution should be covered by this: #327

fkiraly · 2024-05-16T14:58:57Z

If we want to make further changes, these will have to wait until 2.5.0, as we will now proceed to the 2.3.0 release shortly, after which the second next MINOR release will be 2.5.0 - see point 4 here which explains this: https://www.sktime.net/en/stable/developer_guide/deprecation.html#deprecation-policy

The changes in #329 or #327 should not prevent us from making further changes to the distribution interface.

Deprecation messages for `CyclicBoosting` for deprecations and changes in 2.4.0. Alternative to #320. * restores sequence of parameters to sequence in version 2.2.2, to avoid breakage in the 2.3.0 release * adds deprecation message for `bound` parameter * renames new `version` parameter to `dist_type` - the name is a bit misleading, as it will probably imply "version" in the semantic versioning sense to an ordinary user. Does not require deprecation, because added since 2.2.2. Also makes improvements to the docstring of the `CyclicBoosting` estimator. Difference to #320: does not carry out deprecations for distributions, under a working assumption that #327 will be merged.

setoguchi-naoki · 2024-05-19T05:04:02Z

Thank you for your contribution. As there are no tasks left for me to do, I have no problem closing this PR if it is unnecessary.

fkiraly · 2024-05-19T10:08:11Z

Well, I dealt only with the CyclicBoosting class for deprecation.

We may stlil have to decide what to do with the distributions, in deprecation. For instance:

all QPD_X distributions have parameters lower and upper. Are these needed? Or should we deprecate them? I think @FelixWick mentioned this in his review of [ENH] native implementation of Johnson QPD family, explicit pdf #327, and I replied that I left these because they were in the initial implementation.
what about dist_shape?
do we want to allow arbitrary distributions, incl scipy and skpro distributions as base distributions in QPD?

My advice would hence be, let's be clear about the target interfaces, from that it will become clear whether we need any further change/deprecation management, or not.

setoguchi-naoki · 2024-05-20T02:54:37Z

all QPD_X distributions have parameters lower and upper. Are these needed? Or should we deprecate them? I think @FelixWick mentioned this in his review of #327, and I replied that I left these because they were in the initial implementation.

As mentioned in #327 , the lower and upper parameters are not necessarily needed in all QPD classes if findiff is not used. Therefore, the upper parameter can be removed in QPD_S and the lower and upper parameters in QPD_U.

what about dist_shape?

The dist_shape parameter is an option for fine-tuning the QPD_U. Therefore, it can be removed as a deprecated parameter if simplification of the QPD class interface is a priority. How do you think?

do we want to allow arbitrary distributions, incl scipy and skpro distributions as base distributions in QPD?

It would be good to be able to specify scipy and skpro distributions, as you have implemented in #327. In this case, I don't think some support is needed in the deprecated process.

Let me check the situation. Should I add a deplication message to this PR to meet the #327 specification? Is there anything else I should consider?

FelixWick · 2024-05-20T08:10:45Z

The shape parameter is only used for the extended QPD versions (Which include QPD_U. That means only QPD_S and QPD_B are "original" ones.), which I suggest to remove here.

fkiraly · 2024-05-20T10:42:32Z

As mentioned in #327 , the lower and upper parameters are not necessarily needed in all QPD classes if findiff is not used. Therefore, the upper parameter can be removed in QPD_S and the lower and upper parameters in QPD_U.

Ok, that makes sense, I was wondering what these were used for, already.

The need to use findiff is completely removed now, since the current native implementation computes pdf explicitly (just use the chain rule 😁 ). So let's deprecate the unused parameters then.

The dist_shape parameter is an option for fine-tuning the QPD_U. Therefore, it can be removed as a deprecated parameter if simplification of the QPD class interface is a priority. How do you think?

I agree it should be removed. I would actually replace it by:

renaming version to a more appropriate name, e.g., base_dist
allowing base_dist to be a parametric skpro distirbution, which can have its own parameters. If users want to pass dist_shape, they can pass it as a parameter to base_dist.

fkiraly · 2024-05-20T10:43:53Z

Let me check the situation. Should I add a deplication message to this PR to meet the #327 specification? Is there anything else I should consider?

This might require extending the logic, at least addition of a test case that passes an skpro distirbution.

I would also strongly suggest to rename the parameter version, as it has strong connotations with versioning, e.g., semver.

…osting-deprecation-support

review-notebook-app · 2024-05-21T16:04:10Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

setoguchi-naoki · 2024-05-21T16:19:02Z

@fkiraly
I modfied deplication message like

Removal of uppers and dist_shape from some QPD classes
Rename version to base_dist

at least addition of a test case that passes an skpro distirbution.

The skpro distribution has not yet been introduced and tested in qpd.py. Should it be done in other branches?

fkiraly · 2024-05-22T00:20:52Z

Great!

I think you're quite out of sync with your branch since the new release - can you merge main into your branch, and ensure to resolve all conflicts? Only the files related to CyclicBootsing should be changed.

fkiraly · 2024-05-22T14:29:22Z

FYI, see

I think you somehow ended up on a conflicting branch - can you kindly sync with main?

setoguchi-naoki · 2024-05-22T14:41:06Z

I think I have resolved the conflicts in the main branch.

fkiraly · 2024-05-22T15:57:31Z

I think I have resolved the conflicts in the main branch.

👍 yes, looks like it!

fkiraly

Great!

One minor thing, we should ensure that users can already move to the future behaviour, i.e., have a chance to update their code in advance. That would mean adding the base_dist parameter already right now, an dif it is passed, it overrides version.

Have a look here how renaming a parameter works, including a suggested warning message: https://www.sktime.net/en/stable/developer_guide/deprecation.html#id2

setoguchi-naoki · 2024-05-26T03:08:32Z

Thank you. Fixes for parameter renaming have been applied.

fkiraly

Made some minor tweaks around the docstrings (moved to the "final" form), and release manager actions.

add deplication warnings

a2184b8

setoguchi-naoki mentioned this pull request May 12, 2024

[ENH] update Johnson QPDistributions with bugfixes and vectorization (cyclic-boosting ver.1.4.0) #232

Merged

5 tasks

apply pre-commit

0dc636c

fkiraly changed the title ~~[MNT] Deplication announcement for CyclicBoosting~~ [MNT] Deprecation message for CyclicBoosting changes May 12, 2024

fkiraly added maintenance Continuous integration, unit testing & package distribution module:regression probabilistic regression module labels May 12, 2024

fkiraly reviewed May 12, 2024

View reviewed changes

fkiraly requested changes May 12, 2024

View reviewed changes

mod message

8be682f

fkiraly mentioned this pull request May 15, 2024

[MNT] deprecation handling for CyclicBoosting #329

Merged

specified 2.5.0

4f77ccd

setoguchi-naoki added 2 commits May 22, 2024 01:02

Merge branch 'main' of https://github.com/sktime/skpro into cyclic-bo…

e435bee

…osting-deprecation-support

update deplication message

557dad0

Merge branch 'main' into cyclic-boosting-deprecation-support

922ee6a

mod docstring and param

edf77b6

fkiraly requested changes May 22, 2024

View reviewed changes

add base_dist parameter.

8b57aae

fkiraly added 2 commits May 28, 2024 00:27

Merge branch 'main' into pr/320

8353525

update deprecation logic

436f53f

fkiraly approved these changes May 28, 2024

View reviewed changes

fkiraly merged commit 5198603 into sktime:main May 28, 2024
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNT] Deprecation message for `CyclicBoosting` changes #320

[MNT] Deprecation message for `CyclicBoosting` changes #320

setoguchi-naoki commented May 12, 2024

fkiraly May 12, 2024

setoguchi-naoki May 14, 2024

fkiraly left a comment

setoguchi-naoki commented May 14, 2024

setoguchi-naoki commented May 14, 2024

fkiraly commented May 15, 2024 •

edited

Loading

fkiraly commented May 16, 2024 •

edited

Loading

setoguchi-naoki commented May 19, 2024

fkiraly commented May 19, 2024

setoguchi-naoki commented May 20, 2024

FelixWick commented May 20, 2024

fkiraly commented May 20, 2024

fkiraly commented May 20, 2024

review-notebook-app bot commented May 21, 2024

setoguchi-naoki commented May 21, 2024

fkiraly commented May 22, 2024

fkiraly commented May 22, 2024

setoguchi-naoki commented May 22, 2024

fkiraly commented May 22, 2024

fkiraly left a comment

setoguchi-naoki commented May 26, 2024

fkiraly left a comment

[MNT] Deprecation message for CyclicBoosting changes #320

[MNT] Deprecation message for CyclicBoosting changes #320

Conversation

setoguchi-naoki commented May 12, 2024

Reference Issues/PRs

What does this implement/fix? Explain your changes.

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

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Any other comments?

PR checklist

For all contributions

For new estimators

fkiraly May 12, 2024

Choose a reason for hiding this comment

setoguchi-naoki May 14, 2024

Choose a reason for hiding this comment

fkiraly left a comment

Choose a reason for hiding this comment

setoguchi-naoki commented May 14, 2024

setoguchi-naoki commented May 14, 2024

fkiraly commented May 15, 2024 • edited Loading

fkiraly commented May 16, 2024 • edited Loading

setoguchi-naoki commented May 19, 2024

fkiraly commented May 19, 2024

setoguchi-naoki commented May 20, 2024

FelixWick commented May 20, 2024

fkiraly commented May 20, 2024

fkiraly commented May 20, 2024

review-notebook-app bot commented May 21, 2024

setoguchi-naoki commented May 21, 2024

fkiraly commented May 22, 2024

fkiraly commented May 22, 2024

setoguchi-naoki commented May 22, 2024

fkiraly commented May 22, 2024

fkiraly left a comment

Choose a reason for hiding this comment

setoguchi-naoki commented May 26, 2024

fkiraly left a comment

Choose a reason for hiding this comment

[MNT] Deprecation message for `CyclicBoosting` changes #320

[MNT] Deprecation message for `CyclicBoosting` changes #320

fkiraly commented May 15, 2024 •

edited

Loading

fkiraly commented May 16, 2024 •

edited

Loading