feat(doctest): SSIM metric doctest by jeffydc · Pull Request #2241 · pytorch/ignite

jeffydc · 2021-10-04T05:05:40Z

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)

vfdev-5 · 2021-10-04T05:21:57Z

ignite/metrics/ssim.py

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


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

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

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.

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

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

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

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 ?

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 · 2021-10-06T12:03:05Z

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

jeffydc · 2021-10-06T12:04:45Z

@sdesrozis https://deploy-preview-2241--pytorch-ignite-preview.netlify.app/generated/ignite.metrics.ssim

sdesrozis · 2021-10-06T13:25:27Z

The current rendering

sdesrozis · 2021-10-06T13:27:11Z

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

.. testoutput::
    :hide:

    0.9218971...

sdesrozis · 2021-10-06T13:29:12Z

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...

sdesrozis · 2021-10-06T13:31:01Z

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 !

jeffydc · 2021-10-06T14:35:28Z

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

sdesrozis

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

sdesrozis · 2021-10-12T05:20:09Z

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

vfdev-5 · 2021-10-14T07:42:35Z

@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!

jeffydc · 2021-10-14T07:52:28Z

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

sdesrozis · 2021-10-14T08:26:45Z

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 · 2021-10-14T09:06:14Z

@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!

github-actions bot added ci CI docs module: metrics Metrics module labels Oct 4, 2021

jeffydc force-pushed the doctest/metrics/ssim branch from c3a9ac7 to d45a576 Compare October 4, 2021 05:20

vfdev-5 reviewed Oct 4, 2021

View reviewed changes

feat(doctest): SSIM doctest

4406826

jeffydc force-pushed the doctest/metrics/ssim branch from d45a576 to 4406826 Compare October 4, 2021 05:43

jeffydc added 5 commits October 4, 2021 12:18

fix: install katex

75d0705

fix: ... for expected output

f97eb98

Merge branch 'master' into doctest/metrics/ssim

307f203

Merge branch 'master' into doctest/metrics/ssim

b32c37e

fix: use .. testcode:: & .. testoutput::

3671d5e

jeffydc requested review from sdesrozis and vfdev-5 October 6, 2021 11:44

jeffydc added 2 commits October 7, 2021 11:16

Merge branch 'master' into doctest/metrics/ssim

a075c1c

Merge branch 'master' into doctest/metrics/ssim

3eb33e1

sdesrozis approved these changes Oct 10, 2021

View reviewed changes

Merge branch 'master' into doctest/metrics/ssim

78f4012

Merge branch 'master' into doctest/metrics/ssim

a67072f

This was referenced Oct 14, 2021

[docs] Add doctests for metrics #2265

Closed

[docs] Add doctests for parameter schedulers #2266

Closed

Merge branch 'master' into doctest/metrics/ssim

b4ae864

sdesrozis merged commit a967d87 into pytorch:master Oct 15, 2021

wanjuntham mentioned this pull request Oct 19, 2021

Average metrics doctest #2276

Closed

3 tasks

jeffydc deleted the doctest/metrics/ssim branch June 11, 2022 06:56

Uh oh!

Conversation

jeffydc commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vfdev-5 Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

jeffydc Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vfdev-5 Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

jeffydc Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

sdesrozis Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

jeffydc Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdesrozis Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffydc Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

sdesrozis commented Oct 6, 2021

Uh oh!

jeffydc commented Oct 6, 2021

Uh oh!

sdesrozis commented Oct 6, 2021

Uh oh!

sdesrozis commented Oct 6, 2021

Uh oh!

sdesrozis commented Oct 6, 2021

Uh oh!

sdesrozis commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffydc commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdesrozis left a comment

Choose a reason for hiding this comment

Uh oh!

sdesrozis commented Oct 12, 2021

Uh oh!

vfdev-5 commented Oct 14, 2021

Uh oh!

jeffydc commented Oct 14, 2021

Uh oh!

sdesrozis commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vfdev-5 commented Oct 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeffydc commented Oct 4, 2021 •

edited

Loading

jeffydc Oct 4, 2021 •

edited

Loading

jeffydc Oct 6, 2021 •

edited

Loading

sdesrozis Oct 6, 2021 •

edited

Loading

sdesrozis commented Oct 6, 2021 •

edited

Loading

jeffydc commented Oct 6, 2021 •

edited

Loading

sdesrozis commented Oct 14, 2021 •

edited

Loading