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

[ENH] Add sphinx event to add capability table to estimators' docs individually #2468

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

inclinedadarsh
Copy link
Contributor

@inclinedadarsh inclinedadarsh commented Dec 22, 2024

Reference Issues/PRs

Fixes #1430

What does this implement/fix? Explain your changes.

Add html-page-context callback event to add capabilities table individually to each estimators' docs

  • implement _add_estimator_capabilities_table function
  • use that function with html-page-context sphinx event callback

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

No, it doesn't.

PR checklist

For all contributions
  • I've added myself to the list of contributors. Alternatively, you can use the @all-contributors bot to do this for you.
  • The PR title starts with either [ENH], [MNT], [DOC], [BUG], [REF], [DEP] or [GOV] indicating whether the PR topic is related to enhancement, maintenance, documentation, bugs, refactoring, deprecation or governance.

Sorry, something went wrong.

@aeon-actions-bot aeon-actions-bot bot added the enhancement New feature, improvement request or other non-bug code enhancement label Dec 22, 2024
@aeon-actions-bot
Copy link
Contributor

Thank you for contributing to aeon

I have added the following labels to this PR based on the title: [ enhancement ].

The Checks tab will show the status of our automated tests. You can click on individual test runs in the tab or "Details" in the panel below to see more information if there is a failure.

If our pre-commit code quality check fails, any trivial fixes will automatically be pushed to your PR unless it is a draft.

Don't hesitate to ask questions on the aeon Slack channel if you have any.

PR CI actions

These checkboxes will add labels to enable/disable CI functionality for this PR. This may not take effect immediately, and a new commit may be required to run the new configuration.

  • Run pre-commit checks for all files
  • Run mypy typecheck tests
  • Run all pytest tests and configurations
  • Run all notebook example tests
  • Run numba-disabled codecov tests
  • Stop automatic pre-commit fixes (always disabled for drafts)
  • Disable numba cache loading
  • Push an empty commit to re-run CI checks

Sorry, something went wrong.

@inclinedadarsh
Copy link
Contributor Author

@all-contributors please add @inclinedadarsh for code

Copy link
Contributor

@inclinedadarsh

I've put up a pull request to add @inclinedadarsh! 🎉

@baraline
Copy link
Member

Seems like the issue was about commit from your side somehow, as merging main fixed it.

One last bit that we should resolve is to have uniform format between what your script generate and the format in the files where capabilities are already listed. I think the simplest would be to remove the existing ones and only keep those generated by the script. Would that be OK for you @SebastianSchmidl ?

@SebastianSchmidl
Copy link
Member

Seems like the issue was about commit from your side somehow, as merging main fixed it.

One last bit that we should resolve is to have uniform format between what your script generate and the format in the files where capabilities are already listed. I think the simplest would be to remove the existing ones and only keep those generated by the script. Would that be OK for you @SebastianSchmidl ?

Sure, the more automation the better 👍

@inclinedadarsh
Copy link
Contributor Author

This sounds good.

Should I remove all the capabilities table from AD modules that were added manually @baraline ?

@baraline
Copy link
Member

Yes please :)

@SebastianSchmidl
Copy link
Member

Some information in the manually added AD tables is not captured via tags, e.g.

       * - Input data format
         - univariate
       * - Output data format
         - anomaly scores ◀️ **not in tags**
       * - Learning Type
         - unsupervised ◀️ **not in tags**

I think, we should move this information to the tags then. We just do not have a standard way to do this currently.

I would propose the following:

  • for the learning type, we use one-hot-encoding similar to the other capabilities (a single method can have multiple supported learning types)
  • for the output data format, we just use a single string-tag

E.g. for DWT-MLEAD:

    _tags = {
        "capability:univariate": True,
        "capability:multivariate": False,
        "capability:missing_values": False,
        "capability:unsupervised": True,
        "capability:semi-supervised": False,
        "capability:supervised": False,
        "output_format: "anomaly scores"
    }

@MatthewMiddlehurst is this compatible with the original design of the tags and the other modules?

@MatthewMiddlehurst
Copy link
Member

Really like this as a start. Resolves a lot of issues that come with documenting estimator tags and a good base to build on potentially. I wanted to put together a short/medium GSoC project for documentation items like this so may include the PR as an example in our project page.

@SebastianSchmidl single module and information-only tags are fine. I don't see anything wrong with the ones suggested there, other than maybe names if its AD only,

@inclinedadarsh
Copy link
Contributor Author

Heyy, following this conversation, I think I should remove all the manually added tables in the AD modules, and the redesigning the api or adding more information in tags should be done in a newer issue.

Please confirm @baraline @MatthewMiddlehurst if this is correct, if yes then I'll push a change removing all the manually added tags and checking if everything is building correctly.

Thank you!

@baraline
Copy link
Member

Correct ✅

@SebastianSchmidl
Copy link
Member

Works for me.

If you remove the output format and learning type information from the documentation in the AD module and not add them to the tags, please open a new issue that this information was removed and needs to be added back somehow. Thank you!

Verified

This commit was signed with the committer’s verified signature.
JLLeitschuh Jonathan Leitschuh
- add `html-page-context` callback event to add capabilities table individually to each estimators' docs
@inclinedadarsh
Copy link
Contributor Author

Hey @SebastianSchmidl & @MatthewMiddlehurst and everyone else, I have created a new issue to add the learning type and output format information in the AD module at #2490

Please check it out.

I'll proceed to remove the capabilities table in the AD module

@inclinedadarsh
Copy link
Contributor Author

Please review the PR and let me know if anymore changes are required.

Also @MatthewMiddlehurst, I'd love to know how I can participate in GSoC regarding the same (#2468 (comment)). Please let me know how I can prepare better for GSoC if possible.

Copy link
Member

@SebastianSchmidl SebastianSchmidl left a comment

Choose a reason for hiding this comment

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

Very nice work 👍🏼 I checked some generated HTML-pages, and they contain the capabilities table as expected.

I just have some minor (code-style) requests.

docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
@inclinedadarsh
Copy link
Contributor Author

Yes, these absolutely make the file more readable. Thank you for your suggestions, @SebastianSchmidl!
I have made the changes. Please review and let me know.

Thank you!

Copy link
Contributor

@TonyBagnall TonyBagnall left a comment

Choose a reason for hiding this comment

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

excellent, thank you

@TonyBagnall TonyBagnall merged commit f17762e into aeon-toolkit:main Jan 14, 2025
16 checks passed
@MatthewMiddlehurst
Copy link
Member

@all-contributors please add @inclinedadarsh for code

Copy link
Contributor

@MatthewMiddlehurst

@inclinedadarsh already contributed before to code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, improvement request or other non-bug code enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Add individual estimator capabilities in the documentation
5 participants