[ENH] ensure that all estimators have two test parameter sets #3429
Comments
fkiraly
added
good first issue
Towards #3429. This adds a second parameter set for all estimators checked via `check_estimator` in the `no-softdeps` CI element. This is generally useful, and also allows #2862 to pass that CI element. Also fixes a bug discovered through this: `ExponentTransformer.inverse_transform` breaking if `power` is close to zero. This is now dealt with by a skip and a warning.
|
Hi @fkiraly I’d like to to tackle this if it’s ok for you. |
|
sure, @Abelarm - pick an estimator! |
…gressorPipeline` (#3857) `set_params` bug in `ClassifierPipeline` and `RegressorPipeline` was broken, it would not correctly update parameters. The failure to detect this is an instance of the known problem #3429 The bug has been fixed, and is now covered by appropriate tests (addition of a second parameter set). The fix is as follow: * the issue was in `set_params` which accidentally had one too many layer of nesting in the param dict indexing, e.g., `classifier__` etc. This would materialize only for doubly nested estimators. * this was fixed, with a concomitant extension of the dict subset utility in the `_HetereogenousMetaEstimator`. Depends on #3858 as the `DummyClassifier` is used as one of two param sets.
…ts per estimator to 2 or larger (#4043) This PR adds test parameter sets to some estimators which have only one, towards issue #3429. This ensures that the test suite can properly test set and reset of parameters, and increases test coverage by ensuring that more parameters have more than one value setting in the tests. Test that detects estimators with only one parameter set: #2862 (related, not a dep) Depends on fixes of bugs detected through the new parameter sets: * #4047 * #4049 * #4057
…ts per estimator to 2 or larger (sktime#4043) This PR adds test parameter sets to some estimators which have only one, towards issue sktime#3429. This ensures that the test suite can properly test set and reset of parameters, and increases test coverage by ensuring that more parameters have more than one value setting in the tests. Test that detects estimators with only one parameter set: sktime#2862 (related, not a dep) Depends on fixes of bugs detected through the new parameter sets: * sktime#4047 * sktime#4049 * sktime#4057
Towards #3429, test parameter sets for performance metrics.
|
I am picking SARIMAX. |
|
which estimators are still left? |
|
@julia-kraus, the failures in this diagnostic PR #2862 correspond to the ones that do have only one - it might not be 100% up to date, I'll restart it so it is: |
|
Hi , can I take up this issue for the estimator: "TimeSeriesForestClassier" |
|
@namita0210, absolutely! All yours! |
#### What does this implement/fix? Explain your changes. <!-- A clear and concise description of what you have implemented. --> Implemented the standard 'get_test_params' class method with the appropriate docstring and applicable parameters. Added a couple test params for `RNNNetwork` contributing towards #3429. One test param that covers the default set and another that covers the 'units' parameter.
Towards #3429 Adds a second test parameter set for shapeDTW
|
Hi, I will try to tackle the MatrixProfileClassifier. @fkiraly |
|
great, thanks, @MMTrooper! |
|
Hi @fkiraly, I'm currently working on adding new test parameter sets for the estimators identified in the issue. I'll be focusing on Thanks! |
) - Introduce two test parameter sets for ``TimeSeriesKMeansTslearn`` in the ``get_test_params`` function. - Reference Issues Towards : #3429 - Tests passed: pytest sktime\clustering\tests\test_k_means.py
|
Hello @fkiraly , I will try to work on the estimator:- LogTransformer |
|
Hi @fkiraly , I will try to work on |
This PR enforces a stricter condition on `get_test_params`, namely that it should always run, even if all sensible instances require soft dependencies. This is to make the inspection contracts simpler and unconditional as regards dependencies. Two instances where this has recently caused problems is the `TemporianTransformer` in #5956 (FYI @ianspektor, @achoum, @javiber), and the #5880 (FYI @benHeid, @astrogilda). Having breaking `get_test_params` will also prevent the code snippet in the entry issue #3429 from running, which is causing problems from new contributors, as that issue is presented as a simple entry task. The code snippet is now covered by guaranteeing that `get_test_params` always runs. Includes: fix for `TSBootstrapAdapter`, which was the only non-compliant estimator.
… and non-precomputed mode to improve memory efficiency (#6217) #### Reference Issues/PRs #3429 (comment) #### What does this implement/fix? Explain your changes. adds test parameter sets for `KNeighborsTimeSeriesRegressor`; adds support for non-brute algorithms and non-precomputed mode, mirroring #5937
|
Hello @fkiraly, Thank you!! |
|
do you mean, you would like to work on |
|
@fkiraly, Oh I'm sorry about that! I meant if I could work on this! I wrote that while traveling, so sorry again! |
|
Sure! No worries, and thanks for contributing! For this estimator, kindly be aware of:
|
|
@fkiraly, Thank you so much for letting me help out! |
#### What does this implement/fix? Explain your changes. Towards #3429 I decided to add the estimator parameter and set it to the scikit-learn classifier `KNeighborsClassifier`. I also added my self as a contributor. Let me know if it was appropriate or I need to make a better implementation.
|
Hello @fkiraly, I hope that you're having a good start to your week! I wanted to let you know that I added 2 test parameters for the I am really excited to test out the test parameters and getting your feedback! I can try to test out the changes locally first if that is the preferred protocol. I learned a lot about LSTMs in the context of time series from this. I would really love to possibly contribute more after this estimator's parameters are completed if that is okay. Thank you so much again! |
|
Great, @shlok191! I recommend you open a pull request, where core developers can discuss your contribution further and possiby merge it! |
|
@fkiraly, I just added a PR. Thanks a lot again for letting me contribute! 😃 |
|
Thanks for your contribution! |
We should ensure that all estimators (that have parameters) possess at least two test parameter sets.
The two (or more) parameter sets should:
fitis the bottleneck (so we should not overdo it with too many parameter sets)Recipe:
get_test_paramsimplemented, orget_test_paramsreturning only a single dictionary instead of a list of two (or more) dictionaries.An example PR that adds second parameter sets for some estimators can be found here: #3428
Finding some estimators that have only one parameter set can also be done speedily by using this PR #2862 which adds a test for two parameter sets - either run the test suite from the branch locally, or look into the failing CI.
Locally running code which does this:
Current output:
AggregatorBaggingForecasterCNNNetworkClaSPTransformerColumnEnsembleClassifierColumnTransformerColumnwiseTransformerComposableTimeSeriesForestRegressorConstraintViolationCutoffSplitterDOBINDWTTransformerDistFromAlignerDistanceFeaturesDontUpdateDummyRegressorEmpiricalCoverageExpandingWindowSplitterFCNNetworkFeatureSelectionFilterFinancialHolidaysTransformerFittedParamExtractorGeometricMeanAbsoluteErrorGeometricMeanRelativeAbsoluteErrorGreedyGaussianSegmentationHCrystalBallAdapterHOG1DTransformerHampelFilterHolidayFeaturesInceptionTimeNetworkIndividualBOSSIndividualTDEKNeighborsTimeSeriesRegressorKalmanFilterTransformerFPKalmanFilterTransformerPKLSTMFCNNetworkLogTransformerMACNNNetworkMCDCNNClassifierMCDCNNNetworkMCDCNNRegressorMLPNetworkMatrixProfileMatrixProfileClassifierMatrixProfileTransformerMeanAbsoluteErrorMeanRelativeAbsoluteErrorMedianAbsoluteErrorMedianRelativeAbsoluteErrorMiniRocketMiniRocketMultivariateMiniRocketMultivariateVariableMultiRocketMultiRocketMultivariateOnlineEnsembleForecasterPAAPCATransformerPaddingTransformerParamFitterPipelinePlateauFinderPluginParamsForecasterPoissonHMMPyODAnnotatorRNNNetworkRandomIntervalFeatureExtractorRandomIntervalSegmenterRandomIntervalsRandomSamplesAugmenterReducerTransformResNetNetworkRocketRocketClassifierRocketRegressorSTRAYShapeDTWShapeletTransformSingleWindowSplitterSlidingWindowSegmenterSlidingWindowSplitterSlopeTransformerStackingForecasterSupervisedTimeSeriesForestTSInterpolatorTapNetNetworkTestPlusTrainSplitterThetaLinesTransformerThetaModularForecasterTimeBinnerTimeSeriesForestClassifierTimeSeriesForestRegressorTimeSeriesKMeansTslearnTimeSeriesLloydsTruncationTransformerUnobservedComponentsWhiteNoiseAugmenterYtoXThe text was updated successfully, but these errors were encountered: