Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DOC] Compose and deep learning classifier doc tidy #3756

Merged
merged 3 commits into from Nov 11, 2022

Conversation

TonyBagnall
Copy link
Contributor

fixes the docstring references and a couple of formatting issues in these packages

@TonyBagnall TonyBagnall added documentation Documentation & tutorials module:classification classification module: time series classification labels Nov 10, 2022
Copy link
Contributor

@MatthewMiddlehurst MatthewMiddlehurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Difficult to tell as these aren't on the webpage, but just from the docstring these look like an improvement.

Could possibly add defaults to some of the parameters, i.e.

estimators : list of tuples
        List of (name, estimator, column(s)) tuples specifying the transformer
        objects to be applied to subsets of the data.

does not currently display its default.

@TonyBagnall
Copy link
Contributor Author

Difficult to tell as these aren't on the webpage, but just from the docstring these look like an improvement.

Could possibly add defaults to some of the parameters, i.e.

estimators : list of tuples
        List of (name, estimator, column(s)) tuples specifying the transformer
        objects to be applied to subsets of the data.

does not currently display its default.

It doesnt actually have a default, it seems to be a required parameter. I dont want to change the structure of this here

@TonyBagnall
Copy link
Contributor Author

@achieveordie @AurumnPegasus are these changes ok with you?

@MatthewMiddlehurst
Copy link
Contributor

Oh, yes, it's the column ensemble so it requires estimators as a parameter. All good.

@TonyBagnall TonyBagnall merged commit fe751d5 into main Nov 11, 2022
@TonyBagnall TonyBagnall deleted the classifier-doc-tidy branch November 11, 2022 13:31
@TonyBagnall
Copy link
Contributor Author

ah sorry, meant to wait to hear from @achieveordie @AurumnPegasus, please revert if necessary (I am doing something with the tests so didnt want to clash with #3761)

@achieveordie
Copy link
Collaborator

All changes definitely improve the documentation. Seems like I was late only by a small margin :)

@TonyBagnall
Copy link
Contributor Author

thanks, not at all late, I was too hasty!

@AurumnPegasus
Copy link
Contributor

I see it has been merged, but yes, the changes look good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation & tutorials module:classification classification module: time series classification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants