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

Added links to all the metric files #2474

Merged
merged 4 commits into from
Feb 19, 2022
Merged

Conversation

sayantan1410
Copy link
Contributor

Fixes #2437
Opened this PR as an extension of this PR #2468, I by mistake lost track of that branch. Apology or the inconvenience. Extremely sorry.
@vfdev-5 @DevPranjal Kindly let me know if the changes are alright for you or I have made some mistake.

Check list:

  • Documentation is updated (if required)

@github-actions github-actions bot added the docs label Feb 17, 2022
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 17, 2022

@sayantan1410 thanks for the PR !

Actually, I think we decided a bit too fast about including this text to defaults.rst as now it is associated with metrics, however, we are also using it for certain handlers:

https://deploy-preview-2474--pytorch-ignite-preview.netlify.app/generated/ignite.handlers.param_scheduler.create_lr_scheduler_with_warmup.html#ignite.handlers.param_scheduler.create_lr_scheduler_with_warmup

@sayantan1410 @DevPranjal Any ideas how to deal with that ?

@sayantan1410
Copy link
Contributor Author

sayantan1410 commented Feb 17, 2022

@vfdev-5 There are two solutions in my mind :

  1. Either to add the text to all the metric docstring individually, basically the one we rejected earlier.
  2. To create another rst file like default.rst and include it in only the metric docstrings. And remove the previous default.rst from the metric docstrings.
    The default.rst will not have the text while the default_metric.rst will have the text and rest everything will be similar.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 17, 2022

In Idea (2) there could be few difficulties like those we had with defaults.rst (we have to copy it into a specific path to be discovered), we have to ensure that python definitions aren't duplicated and we still have to update all docstring for metrics to rename defaults.rst -> default_metric.rst.

Idea (1) is more explicit but can be a pain to do...

@sayantan1410
Copy link
Contributor Author

@vfdev-5 Not really, I will just have to copy and paste the text to all the metric docstrings !!
Let's wait for @DevPranjal 's thoughts and then we can move forward with this I think.

@DevPranjal
Copy link
Contributor

With the above issue, Idea 1. sounds good to me

@sayantan1410
Copy link
Contributor Author

@vfdev-5 Hey, I have added the text for all the metric.py files listed here - https://pytorch.org/ignite/metrics.html#complete-list-of-metrics. Let me know if I have missed any.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 18, 2022

@sayantan1410 I provided a temporary CI fix: #2477
Once it is merged please sync your PR with master and we can land it.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @sayantan1410

@vfdev-5 vfdev-5 enabled auto-merge (squash) February 18, 2022 18:52
@sayantan1410
Copy link
Contributor Author

@vfdev-5 hey, some checks were unsuccessful that's why it did not merge, can you check once why ??

@vfdev-5 vfdev-5 disabled auto-merge February 19, 2022 15:37
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 19, 2022

@sayantan1410 yes, there is an issue with new release of openai gym and deprecated env cartpole. we have to fix it.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 19, 2022

I think I can merge your PR as failures are unrelated.

@vfdev-5 vfdev-5 merged commit 6d385eb into pytorch:master Feb 19, 2022
@sayantan1410
Copy link
Contributor Author

@thanks for closing the PR !!

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Feb 19, 2022

@sayantan1410 can you please open an issue describing the failed tests and maybe a fix ?

@sayantan1410
Copy link
Contributor Author

@vfdev-5 yeah sure I will do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an explanation on how metrics work with engine
3 participants