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

feat(doctest): SSIM metric doctest #2241

Merged
merged 11 commits into from
Oct 15, 2021

Conversation

ydcjeff
Copy link
Contributor

@ydcjeff ydcjeff commented Oct 4, 2021

ref #2230

Description:

  • Add doctest for SSIM metric
  • Add doctest gh job
  • Sphinx makes doctest easy by importing modules we need in doctest_global_setup

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

metric = SSIM(data_range=1.0)
metric.attach(engine, "ssim")
>>> def process_function(engine, batch):
... y_pred, y = batch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't it work with code-block ? I'm not a big fan of >>>

Copy link
Contributor Author

@ydcjeff ydcjeff Oct 4, 2021

Choose a reason for hiding this comment

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

It does not work AFAIK. If >>> is not preferred, we need to use .. testcode:: and .. testoutput:: which is more verbose.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With >>> when code is copied it is with these useless >>>. I think it would be better to use other tags instead of >>> which requires each line modification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sphinx-copybutton can be modified to strip >>> in conf.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the @vfdev-5's remark about >>>. Maybe we could explore an alternative and see ?

Copy link
Contributor Author

@ydcjeff ydcjeff Oct 6, 2021

Choose a reason for hiding this comment

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

@vfdev-5 @sdesrozis changed to using .. testcode:: and .. testoutput::. If we are using those, we need to use print to test the expected output.

Copy link
Contributor

@sdesrozis sdesrozis Oct 6, 2021

Choose a reason for hiding this comment

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

If we are using those, we need to use print to test the expected output.

Not sure to understand, sorry.

Does the rendering of the doc show the output ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean if we are using .. testcode:: and .. testoutput::, we need to use print like print(state.metrics['ssim']) to test the expected output.

Does the rendering of the doc show the output ?

Yes, it shows.

@sdesrozis
Copy link
Contributor

@ydcjeff I just would like to see the rendering and that’s fine on my side !

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Oct 6, 2021

@sdesrozis
Copy link
Contributor

The current rendering
Capture d’écran de 2021-10-06 15-14-51

@sdesrozis
Copy link
Contributor

We could hide the output cell adding the option :hide:

.. testoutput::
    :hide:

    0.9218971...

Capture d’écran de 2021-10-06 15-24-10

@sdesrozis
Copy link
Contributor

Or an alternative would be to mix doctest and sphinx extension.

.. testcode::

    def process_function(engine, batch):
        y_pred, y = batch
        return y_pred, y
    engine = Engine(process_function)
    metric = SSIM(data_range=1.0)
    metric.attach(engine, 'ssim')
    preds = torch.rand([4, 3, 16, 16])
    target = preds * 0.75
    state = engine.run([[preds, target]])

.. doctest::

    >>> state.metrics['ssim']
    0.9218971...

Capture d’écran de 2021-10-06 15-24-23

@sdesrozis
Copy link
Contributor

sdesrozis commented Oct 6, 2021

Each examples above are tested by make doctest. I think a section title would be valuable before the output (Expected output(s) ?).

What do you think ? Everything works fine though !

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Oct 6, 2021

Mostly python doctest is used alongside with output. So it's better to show output as well maybe with "Output" section if we want.

Copy link
Contributor

@sdesrozis sdesrozis left a comment

Choose a reason for hiding this comment

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

@ydcjeff Thanks ! Now we can refactor every docstring example.

@sdesrozis
Copy link
Contributor

@vfdev-5 Is it fine for you with this PR ?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 14, 2021

@sdesrozis @ydcjeff I'm OK with this change however can you estimate how much work it would require to rework all doc strings and even if this is feasible for all doc strings ? Will we do a mix of tested and non-tested doc strings ?
Please, update #2230 with more details and maybe make the issue as "help wanted"/Hacktoberfest to bring more contributors on that. Thanks!

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Oct 14, 2021

From the first glance, I think most examples can be tested, but I'm not sure for distributed ones.

@sdesrozis
Copy link
Contributor

sdesrozis commented Oct 14, 2021

First of all, having the metrics and param schedulers tests in the doc validated would be great. Except for idist, I'm not sure we have that many tests distributed in the docstring.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Oct 14, 2021

@sdesrozis @ydcjeff I let you open new tracking issue or update existing one to see clearly the task and how we can involve contributors to that. Sylvain, I let you guide this work and merge this PR or wait for something. Thanks!

@sdesrozis sdesrozis merged commit a967d87 into pytorch:master Oct 15, 2021
@wanjuntham wanjuntham mentioned this pull request Oct 19, 2021
3 tasks
@ydcjeff ydcjeff deleted the doctest/metrics/ssim branch June 11, 2022 06:56
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.

3 participants