[ENH] remove private methods from parameters of `ProximityForest`, `ProximityTree`, and `ProximityStump` #6046

fnhirwa · 2024-03-02T06:21:20Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Removed methods that were being instantiated with the objects of ProximityForest, ProximityTree, and ProximityStump classes and made them the class methods.

For the get_gain method which defaults to gini_gain I added a dictionary to track the gain functions in case there is more gain functions added in the future.

Methods changed

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

What should a reviewer concentrate their feedback on?

Whether the refactor logic is well versed

Did you add any tests for the change?

No tests added

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 sktime 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
Optionally, I've added myself and possibly others to the CODEOWNERS file - do this if you want to become the owner or maintainer of an estimator you added.
See here for further details on the algorithm maintainer role.
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

Nice, thanks!

I was wondering, there are a lot of functions which just dispatch to a function. Should these be moved into the class, or otherwise simplified? (to discuss, not necessarily to action)

fkiraly · 2024-03-02T13:37:35Z

The docstring example of ProximityStump seems to fail.

You can also check the estimator by running tests using sktime.utils.check_estimator.

fnhirwa · 2024-03-02T13:55:26Z

Nice, thanks!

I was wondering, there are a lot of functions which just dispatch to a function. Should these be moved into the class, or otherwise simplified? (to discuss, not necessarily to action)

Well, I think moving these functions to classes would make code more cohesive and enforce readability and maintainability even if it will be achieved at the cost of duplicating some functions here 🙄

fkiraly · 2024-03-02T14:21:41Z

Well, I think moving these functions to classes would make code more cohesive and enforce readability and maintainability even if it will be achieved at the cost of duplicating some functions here

What about moving the entire body to the class then?

fnhirwa · 2024-03-02T14:29:22Z

Well, I think moving these functions to classes would make code more cohesive and enforce readability and maintainability even if it will be achieved at the cost of duplicating some functions here

What about moving the entire body to the class then?

What about moving the entire body to the class then?

Yeah if you agree with it I can start away with it on this PR

fkiraly · 2024-03-02T17:21:33Z

if you agree that´s a good idea, of course. If not, please explain and advise.

…mityStump` moved all function logic to the body and simplified the logic of the functions that were dispatching to other function. Removed redundant logics and tested locally tests are passing

fnhirwa · 2024-03-02T18:26:12Z

if you agree that´s a good idea, of course. If not, please explain and advise.

I agree with it and working on it will push a commit that adds these changes

… fail

Closes sktime#6035 This PR tries to show more reliable coverage badge on README using carryforward flag feature for `complete`. Ref. https://docs.codecov.com/docs/carryforward-flags

Reverts sktime#6041 After the merge, the badge now appears broken and the URL showed no image.

sktime#6038) #### Reference Issues/PRs Part of sktime#3351. See also sktime#3365 #### What does this implement/fix? Explain your changes. Migrated CNTC, InceptionTime, and MACNN regressors to sktime from sktime-dl

@ivarzap

@ivarzap forgot to add author credits in sktime#5942, fixed.

@hazrulakmal

Fixes various minor typos: * docstring typos * author credits: `model_evaluation` was missing credits of @hazrulakmal; module `__init__` should have no authors variable

…ion templates (sktime#6053) This PR adds further clarification regarding immutability of `self`-params in the extension templates, including recipes on handling defaults and estimators (the latter only in "non-simple" extension templates).

Sktime already has an adapter for `neuralforecast` which only implements `RNN` model. RNN's suffer from problems like gradient vanishing while training, So I it's good to have LSTM model implemented that works better than RNN for time-series forecasting. Here is a demo of LSTM implemented in sktime using neuralforecast [notebook](https://colab.research.google.com/drive/1R9z5HS4uRpzVuVYKMHoB-q3-osWgHyD_?usp=sharing) --------- Co-authored-by: Anirban Ray <39331844+yarnabrina@users.noreply.github.com>

fnhirwa · 2024-03-06T07:05:07Z

Hey @fkiraly
I finished refactoring the code and majority of tests are passing but 3 CI runs are failing raising an error
ValueError: Input contains NaN. is this related to my implementation or should I look at the unit tests?
Thanks

fkiraly · 2024-03-06T13:46:21Z

@fnhirwa, the failures are related to the classes you changed, and they are not present on main, which strongly suggests they are caused by your changes.

My gut feeling would be that there might be a small mistake somewhere, e.g., a default value that is different from prior state.

I would advise that you isolate a piece of self-contained code that replicates the error, perhaps a few lines; perhaps using check_estimator. Then, compare traceback of runs on main (without your PR) and on your PR, until you've found a difference (probably a function call where args differ).
Visual studio code debugger or step-wise debug execution would help a lot.

Let us know if you need help, e.g., a demo of how this kind of debugging works.

fnhirwa · 2024-03-06T13:55:43Z

@fnhirwa, the failures are related to the classes you changed, and they are not present on main, which strongly suggests they are caused by your changes.

My gut feeling would be that there might be a small mistake somewhere, e.g., a default value that is different from prior state.

I would advise that you isolate a piece of self-contained code that replicates the error, perhaps a few lines; perhaps using check_estimator. Then, compare traceback of runs on main (without your PR) and on your PR, until you've found a difference (probably a function call where args differ). Visual studio code debugger or step-wise debug execution would help a lot.

Let us know if you need help, e.g., a demo of how this kind of debugging works.

I completely understand that and going to deep dive to find the issue and will push a fix soon🤞

fnhirwa · 2024-03-11T05:34:46Z

@fkiraly the CI tests seems to pass now would like a review from you.
Thanks!

fkiraly · 2024-03-11T14:23:24Z

Sure! Will start remote CI and then review.

sktime/classification/distance_based/_proximity_forest.py

fkiraly

Looks good to me! Refactor points seem correctly moved.

Left some comments regarding docstrings, these are not blocking though.

Test failures are unrelated, see #6094

… functions to docstrings

fnhirwa · 2024-03-12T03:15:19Z

Looks good to me! Refactor points seem correctly moved.

Left some comments regarding docstrings, these are not blocking though.

Test failures are unrelated, see #6094

Just made the requested changes to docstrings and made the gini_gain a default gain function, with the direction of the line to change in case a custom gain function is needed.
Thanks

sktime/classification/distance_based/_proximity_forest.py

fkiraly

Thanks!

fkiraly

(still approve)

fnhirwa added 7 commits March 2, 2024 09:37

made setup_distance_measure a method function

e91b11f

distance getter functions

1d7bf1c

fix docstyle

9651690

refactor exemplars methods

6168fcd

refactor the find_stump method

fde536f

add my username to contributors

2b0cad2

refactor the gain function parameter to the classes

e434175

fnhirwa marked this pull request as ready for review March 2, 2024 06:55

fnhirwa requested review from achieveordie, benHeid, fkiraly and yarnabrina as code owners March 2, 2024 06:55

fkiraly requested changes Mar 2, 2024

View reviewed changes

fixed the ProximityStump docstrings and wrong parameter references

c4d119f

Refactored both classes ProximityForest, ProximityTree and `Proxi…

6a35817

…mityStump` moved all function logic to the body and simplified the logic of the functions that were dispatching to other function. Removed redundant logics and tested locally tests are passing

fnhirwa requested a review from fkiraly March 2, 2024 18:29

fnhirwa and others added 8 commits March 3, 2024 09:32

Fix the NoneType error from test logs

5a5a878

Wrongly referenced methods as distance_measures causing CI tests to…

86c3042

… fail

fixed the tests using check_stimators to ensure they're passing locally

aa2c1b4

[MNT] update codecov configuration and badge URL (sktime#6041)

c634c5d

Closes sktime#6035 This PR tries to show more reliable coverage badge on README using carryforward flag feature for `complete`. Ref. https://docs.codecov.com/docs/carryforward-flags

Revert "[MNT] update codecov configuration and badge URL" (sktime#6049)

8f10b1c

Reverts sktime#6041 After the merge, the badge now appears broken and the URL showed no image.

[ENH] Migrate DL regressors from sktime-dl: CNTC, InceptionTime, MACNN (

ff15e79

sktime#6038) #### Reference Issues/PRs Part of sktime#3351. See also sktime#3365 #### What does this implement/fix? Explain your changes. Migrated CNTC, InceptionTime, and MACNN regressors to sktime from sktime-dl

[DOC] add missing author credits of @ivarzap (sktime#6050)

d8d23ee

@ivarzap forgot to add author credits in sktime#5942, fixed.

[DOC] fix various typos (sktime#6043)

d0d909c

Fixes various minor typos: * docstring typos * author credits: `model_evaluation` was missing credits of @hazrulakmal; module `__init__` should have no authors variable

fkiraly and others added 2 commits March 5, 2024 01:21

fnhirwa added 3 commits March 6, 2024 22:12

fix the nan error in distances calculation

68ff731

Merge remote-tracking branch 'upstream/main' into private-methods

c9b2649

Merge remote-tracking branch 'upstream/main' into private-methods

657938a

fkiraly added module:classification classification module: time series classification enhancement Adding new functionality labels Mar 11, 2024

fkiraly reviewed Mar 11, 2024

View reviewed changes

sktime/classification/distance_based/_proximity_forest.py Outdated Show resolved Hide resolved

fkiraly reviewed Mar 11, 2024

View reviewed changes

sktime/classification/distance_based/_proximity_forest.py Outdated Show resolved Hide resolved

fkiraly previously approved these changes Mar 11, 2024

View reviewed changes

made gini_gain default gain function and added valid distance measure…

6ac52bd

… functions to docstrings

fnhirwa dismissed fkiraly’s stale review via 6ac52bd March 12, 2024 03:13

fnhirwa requested a review from fkiraly March 12, 2024 03:15

fkiraly reviewed Mar 12, 2024

View reviewed changes

sktime/classification/distance_based/_proximity_forest.py Outdated Show resolved Hide resolved

fkiraly previously approved these changes Mar 12, 2024

View reviewed changes

fnhirwa added 2 commits March 13, 2024 10:41

Merge remote-tracking branch 'upstream/main' into private-methods

bcfe189

a slight correction of the function docstrings

d77b417

fnhirwa dismissed fkiraly’s stale review via d77b417 March 13, 2024 05:13

fnhirwa requested a review from fkiraly March 13, 2024 05:13

fkiraly previously approved these changes Mar 14, 2024

View reviewed changes

Update .all-contributorsrc

10b9350

fkiraly dismissed their stale review via 10b9350 March 14, 2024 00:12

fkiraly merged commit e362733 into sktime:main Mar 15, 2024
50 of 54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] remove private methods from parameters of `ProximityForest`, `ProximityTree`, and `ProximityStump` #6046

[ENH] remove private methods from parameters of `ProximityForest`, `ProximityTree`, and `ProximityStump` #6046

fnhirwa commented Mar 2, 2024 •

edited

fkiraly left a comment

fkiraly commented Mar 2, 2024

fnhirwa commented Mar 2, 2024

fkiraly commented Mar 2, 2024

fnhirwa commented Mar 2, 2024

fkiraly commented Mar 2, 2024

fnhirwa commented Mar 2, 2024

fnhirwa commented Mar 6, 2024

fkiraly commented Mar 6, 2024

fnhirwa commented Mar 6, 2024

fnhirwa commented Mar 11, 2024

fkiraly commented Mar 11, 2024

fkiraly left a comment

fnhirwa commented Mar 12, 2024

fkiraly left a comment

fkiraly left a comment

[ENH] remove private methods from parameters of ProximityForest, ProximityTree, and ProximityStump #6046

[ENH] remove private methods from parameters of ProximityForest, ProximityTree, and ProximityStump #6046

Conversation

fnhirwa commented Mar 2, 2024 • edited

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 left a comment

Choose a reason for hiding this comment

fkiraly commented Mar 2, 2024

fnhirwa commented Mar 2, 2024

fkiraly commented Mar 2, 2024

fnhirwa commented Mar 2, 2024

fkiraly commented Mar 2, 2024

fnhirwa commented Mar 2, 2024

fnhirwa commented Mar 6, 2024

fkiraly commented Mar 6, 2024

fnhirwa commented Mar 6, 2024

fnhirwa commented Mar 11, 2024

fkiraly commented Mar 11, 2024

fkiraly left a comment

Choose a reason for hiding this comment

fnhirwa commented Mar 12, 2024

fkiraly left a comment

Choose a reason for hiding this comment

fkiraly left a comment

Choose a reason for hiding this comment

[ENH] remove private methods from parameters of `ProximityForest`, `ProximityTree`, and `ProximityStump` #6046

[ENH] remove private methods from parameters of `ProximityForest`, `ProximityTree`, and `ProximityStump` #6046

fnhirwa commented Mar 2, 2024 •

edited