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] Improve Docstring for MAPE Metrics #4563

Merged
merged 12 commits into from
May 12, 2023
Merged

Conversation

hazrulakmal
Copy link
Collaborator

Reference Issues/PRs

Fixes #4457. See also #4303

What does this implement/fix? Explain your changes.

  1. Added adjusted vs symmetric clarifications for sMAPE.
  2. Improved docstring documentation to include a mathematical summary of the metric and additional parameters that the metric class currently supports.

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

No

Any other comments?

This isn't directly related to the PR, but I ran the documentation build locally to confirm that the changes were displayed correctly. During the process, I noticed that it took a while to build the HTML docs. It would be great if this process could be sped up a bit. Maybe consider sphinx-autobuild extension to automatically detect changes in docs with live-reload in the browser during local development.

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Thanks, really great!

I left some small comments, these are not blocking since small, but kindly have a look.

@fkiraly fkiraly added documentation Documentation & tutorials module:metrics&benchmarking metrics and benchmarking modules labels May 8, 2023
@fkiraly fkiraly changed the title [DOC] Improve Docsting for MAPE Metrics [DOC] Improve Docstring for MAPE Metrics May 9, 2023
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Excellent, all good to go!

Non-blocking comment: The absolute bars around fractions render as too short, so I would recommend to use the \left| and \right| commands instead of just | if it is around \frac

@hazrulakmal
Copy link
Collaborator Author

Done!

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Nice! Thanks a lot!

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Apologies for taking the approval back, but it looks like there is now an issue with the examples.

As the r has been added , the backslash for newline (that was already there!) is now causing trouble. Could you kindly check and possibly reformat?

@hazrulakmal
Copy link
Collaborator Author

hazrulakmal commented May 11, 2023

I see. based on sphnix doc, I added r because it seems that sphinx can't detect the backslash for \frac.

Keep in mind that when you put math markup in Python docstrings read by autodoc, you either have to double all backslashes, or use Python raw strings (r"raw").

Didnt realise that it'd affect the \ at the bottom. Thanks for catching it. I've reformatted it now. It should work.

Also, I'm wondering, which test files that run documentation tests for consistency. would be great if I could run the tests locally and detect this sort of problem before pushing the commits.

@fkiraly
Copy link
Collaborator

fkiraly commented May 11, 2023

Also, I'm wondering, which test files that run documentation tests for consistency. would be great if I could run the tests locally and detect this sort of problem before pushing the commits.

I think that's an extremely valid point! Would appreciate if you could open an issue that there should be some guide on how to run the doctests locally. The current guide only covers pytest.

Could you open an issue to add this to docs/source/developer_guide/continuous_integration.rst which has instructions on how to run tests locally? I would add it as a subsection after Full test suite runs via pytest.
(if you do that, feel free to say that you're going to do this, if not I'll do this at some point closer to the release)

Here's from doctest on running the doctests locally: https://docs.python.org/3/library/doctest.html

@fkiraly
Copy link
Collaborator

fkiraly commented May 11, 2023

you still need the r for the TeX backslashes! You just need to reformat the code example so it does not have a line break.

@hazrulakmal
Copy link
Collaborator Author

hazrulakmal commented May 11, 2023

Actually, I reformatted it by removing the line break first. but something like below happened when I rebuilt the doc. Look at (default='uniform_average').
image

the docstring is formatted as below wo the \. Note that ,default='uniform_average must be on the next line because of the maximum line length of 88 characters.

    multioutput : {'raw_values', 'uniform_average'}  or array-like of shape (n_outputs,)
    , default='uniform_average'
        Defines how to aggregate metric for multivariate (multioutput) data.
        If array-like, values used as weights to average the errors.
        If 'raw_values', returns a full set of errors in case of multioutput input.
        If 'uniform_average', errors of all outputs are averaged with uniform weight.

Even if I indent it to align with {'raw_values', 'uniform_average'} or array-like of shape (n_outputs,), it still doesn't work.

is there a way to still use backslash as a continuation line when the docstring is in raw format (when r is placed before the opening quotes)? or any similar trick to ensure there is a continuation line? I cannot find a way to do so.

Sorry, I didn't pick up that the preferred code style uses r instead of a double backslash. I just got the pre-commit works on my local machine. 😅

Also regarding doctest, will open an issue about it soon! but has doctest already been implemented internally, or is the issue meant to be the first step towards implementing it?

Update

I found a suggestion from this closed PR that I can ignore the 88 max length limit with # noqa: E501 at the end of the docstring, is that permissible?

@fkiraly
Copy link
Collaborator

fkiraly commented May 11, 2023

Also regarding doctest, will open an issue about it soon! but has doctest already been implemented internally, or is the issue meant to be the first step towards implementing it?

doctest already runs as part of the CI, it's only about telling people how to run it locally

is there a way to still use backslash as a continuation line when the docstring is in raw format

I don't know... I would just shorten the line

@fkiraly
Copy link
Collaborator

fkiraly commented May 11, 2023

(well, funnily, this got more complicated than thought. What a fiddly thing, that backslash...)

Copy link
Collaborator Author

@hazrulakmal hazrulakmal left a comment

Choose a reason for hiding this comment

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

What should I do with this?

@hazrulakmal
Copy link
Collaborator Author

hazrulakmal commented May 12, 2023

I don't know... I would just shorten the line

the thing is, that sentence explains a list of acceptable parameters for that argument and what the default parameter is. cant really shorten it else, users won't know what to change in that argument to control the function behaviour.

Anyway, I may have missed your requested changes. 😅 Sorry about that. very new to this thing. I guess I missed the opportunity to get coauthor-commit badges.

(well, funnily, this got more complicated than thought. What a fiddly thing, that backslash...)

yeah lol.

@fkiraly
Copy link
Collaborator

fkiraly commented May 12, 2023

I guess I missed the opportunity to get coauthor-commit badges.

Not sure what badge that is?

You can get a doc badge for this in the all-contributorsrc file if you want, though!

@fkiraly
Copy link
Collaborator

fkiraly commented May 12, 2023

I guess I missed the opportunity to get coauthor-commit badges.

Not sure what badge that is?

I see - well, in that case, I took the liberty to commit directly to your branch with an alternative docstring field that does not go over the character limit, this should also get you the badge :-)

@fkiraly fkiraly merged commit ec4879d into sktime:main May 12, 2023
22 checks passed
@hazrulakmal hazrulakmal deleted the metrics branch May 15, 2023 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation & tutorials module:metrics&benchmarking metrics and benchmarking modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] naming of symmetricMAPE vs adjustedMAPE - clarifying docstring
2 participants