Fixed test in test_precision_recall_curve and tests/ignite/contrib/metrics/regression #2511

sayantan1410 · 2022-03-11T05:10:07Z

Related to #2490

I couldn't figure out why the test was passing initially, I probably needed to gather data from all the processes with idist.all_gather. But as per the conversation in the mentioned PR I have made the changes to it. Let me know if anything needs to be I will be happy to fix it.

vfdev-5 · 2022-03-11T10:28:58Z

I couldn't figure out why the test was passing initially, I probably needed to gather data from all the processes with idist.all_gather.

@sayantan1410 PR seems good. However we need to understand why the tests were passing previously. Please fetch all the data and see why it was passing. Thanks

sdesrozis · 2022-03-11T18:21:26Z

I think there are some others tests broken like this one in regression.

sayantan1410 · 2022-03-12T04:49:39Z

@vfdev-5 Yeah trying to look for the reason !!

sayantan1410 · 2022-03-12T04:50:03Z

@sdesrozis Should I change them in this PR only ?

sdesrozis · 2022-03-12T07:53:54Z

@sayantan1410 It would be great if you could look for others corrupted tests and fix them if you find any. I would say it's mainly in contrib/metrics/regression. You will just have to rename the PR if others fixes are done.

Thanks a lot for your help !

sayantan1410 · 2022-03-12T08:50:14Z

Yeah sure will do that.

sayantan1410 · 2022-03-13T14:34:32Z

Probable reason why

ignite/tests/ignite/contrib/metrics/test_precision_recall_curve.py

Line 196 in 2eb3bcf

def _test_distrib_integration(device):

is passing inspite of being wrong :
There is a difference is value between precision, recall, thresholds and sk_precision, sk_recall and sk_thresolds in a distributed configuration, however, the difference was small and pytest.approx was taking care of it so that tests were passing.
Code to reproduce:

import torch
import ignite.distributed as idist
from ignite.exceptions import NotComputableError
from ignite.metrics import EpochMetric
from ignite.contrib.metrics import PrecisionRecallCurve
from sklearn.metrics import precision_recall_curve
from ignite.engine import Engine


rank = idist.get_rank()
torch.manual_seed(12)
device = idist.device()

def _test(n_epochs, metric_device):
    metric_device = torch.device(metric_device)
    n_iters = 80
    size = 151
    offset = n_iters * size
    y_true = torch.randint(0, 2, size=(offset * idist.get_world_size(),)).to(device)
    y_preds = torch.randint(0, 2, size=(offset * idist.get_world_size(),)).to(device)

    def update(engine, i):
          return (
              y_preds[i * size + rank * offset : (i + 1) * size + rank * offset],
              y_true[i * size + rank * offset : (i + 1) * size + rank * offset],
          )
    engine = Engine(update)

    prc = PrecisionRecallCurve(device=metric_device)
    prc.attach(engine, "prc")

    data = list(range(n_iters))
    engine.run(data=data, max_epochs=n_epochs)

    assert "prc" in engine.state.metrics

    precision, recall, thresholds = engine.state.metrics["prc"]

    np_y_true = y_true.cpu().numpy().ravel()
    np_y_preds = y_preds.cpu().numpy().ravel()

    sk_precision, sk_recall, sk_thresholds = precision_recall_curve(np_y_true, np_y_preds)
    print(f"precision: {precision}")
    print(f"recall: {recall}")
    print(f"thresholds: {thresholds}")
    print(f"sk_precision: {sk_precision}")
    print(f"sk_recall: {sk_recall}")
    print(f"sk_thresholds: {sk_thresholds}")

metric_devices = ["cpu"]
if device.type != "xla":
    metric_devices.append(idist.device())
for metric_device in metric_devices:
    for _ in range(1):
        print("new test")
        _test(n_epochs=1, metric_device=metric_device)

Command I used to run the code:

torchrun --nproc_per_node 2 filename.py --backend prefered backend

sayantan1410 · 2022-03-13T14:35:21Z

@vfdev-5 @sdesrozis Hey probably the tests for the metrics in contrib/metrics/regression are fixed, can you check once.

vfdev-5 · 2022-03-14T10:26:48Z

@sayantan1410 your repro code example does not reproduce the issue and does not work with DDP as there is no communication group created.

sayantan1410 · 2022-03-14T17:17:09Z

@vfdev-5

your repro code example does not reproduce the issue and does not work with DDP as there is no communication group created.

Yeah, I realized this now.
Also, can you once review the changes in this PR, it would be helpful for me.

ignite/engine/events.py

tests/ignite/contrib/metrics/test_precision_recall_curve.py

sayantan1410 · 2022-03-20T13:49:45Z

@vfdev-5 @sdesrozis this was the change I was thinking about, please check once.

vfdev-5 · 2022-03-20T15:19:53Z

tests/ignite/contrib/metrics/regression/test_median_absolute_error.py

+
+        offset = n_iters * size
+        y_true = torch.rand(size=(offset * idist.get_world_size(),)).to(device)
+        y_preds = torch.rand(size=(offset * idist.get_world_size(),)).to(device)


@sayantan1410 either revert these modifications or make them coherent to others and rename the PR.

Yeah, making them coherent with others, also some tests are failing, fixing them as well

Have you updated others ? Do not forget to rename the PR title to reflect that you change not only test_precision_recall_curve.py

Yes, in the other tests this has been changed and I have changed the PR title as well.

Why don't you add

rank = idist.get_rank() torch.manual_seed(rank)

here and other regression files ?

I added this to precision_recall_curve and was waiting for all the tests to pass for it. Now I will add it to the other files as well.

tests/ignite/contrib/metrics/test_precision_recall_curve.py

sayantan1410 · 2022-03-20T16:12:48Z

@vfdev-5 yeah, now I understand it better than at that time :)
Also, what to do to make the tests uniform ?

sayantan1410 · 2022-03-21T08:05:04Z

@vfdev-5 Hey, Any idea why the hvd test is failling, other tests are passing now.

vfdev-5 · 2022-03-21T10:51:52Z

@sayantan1410 looks like majority of tests are failing and not only hvd... Check

sayantan1410 · 2022-03-21T12:41:59Z

@vfdev-5 Hey, that is of the old tests, I have made a commit after that and it is showing me that the tests are passing except for hvd. Probably you need to refresh the page once.

vfdev-5 · 2022-03-21T12:44:17Z

Oh, I see, sorry. I rerun the tests, output looks strange

vfdev-5 · 2022-03-21T12:46:14Z

tests/ignite/contrib/metrics/regression/test_median_relative_absolute_error.py

@@ -186,7 +188,7 @@ def update(engine, i):
        e = np.abs(np_y_true - np_y_preds) / np.abs(np_y_true - np_y_true.mean())
        np_res = np.median(e)

-        assert pytest.approx(res) == np_res
+        assert pytest.approx(res, rel=1e-3) == np_res


Why you up the rel tolerance ? If it starts failing than there can be something in the code as well.

Yes, there can be, but the values were slightly different for all the tests, I looked into the code but couldn't figure out a reason so thought of changing the tolerance.

vfdev-5 · 2022-03-21T12:46:58Z

tests/ignite/contrib/metrics/regression/test_median_absolute_error.py

+
+        offset = n_iters * size
+        y_true = torch.rand(size=(offset * idist.get_world_size(),)).to(device)
+        y_preds = torch.rand(size=(offset * idist.get_world_size(),)).to(device)


Have you updated others ? Do not forget to rename the PR title to reflect that you change not only test_precision_recall_curve.py

vfdev-5 · 2022-03-21T13:11:45Z

tests/ignite/contrib/metrics/regression/test_median_absolute_error.py

@@ -185,7 +187,7 @@ def update(engine, i):
        e = np.abs(np_y_true - np_y_preds)
        np_res = np.median(e)

-        assert pytest.approx(res) == np_res
+        assert pytest.approx(res, rel=1) == np_res


Same remark here, relative error as 1 is too high !

Yeah I also thought so, I am trying to find why there is this difference in values.

sayantan1410 · 2022-03-23T06:45:05Z

@vfdev-5 Hey, Getting assertion error if I am not increasing the tolerance, Any idea how should I fix this ?

vfdev-5 · 2022-04-09T15:36:19Z

@sayantan1410 can we split this PR such that improved tests that are passing could be merged and those are not passing we could investigate case by case. Otherwise you have a stuck block of modifications...

sayantan1410 · 2022-04-09T16:55:31Z

@vfdev-5 I could do that, basically, only the precision_recall_curve is working, if possible can we have a meet once, I can show you where I am stuck, and we can solve the issue may be.

vfdev-5 · 2022-09-04T15:58:47Z

Closing this PR in favor of #2655. Thanks for your work @sayantan1410 !

Fixed a test in test_precision_recall_curve.py

ea71d42

sayantan1410 added 3 commits March 13, 2022 14:51

Fixed integration tests

5c6d863

fixed mypy error

080c49c

fixed pytest.approx relative errors

76fd0f7

sayantan1410 mentioned this pull request Mar 14, 2022

One test for some of the metrics in contrib/metrics needs improvement #2517

Closed

4 tasks

vfdev-5 reviewed Mar 14, 2022

View reviewed changes

ignite/engine/events.py Outdated Show resolved Hide resolved

tests/ignite/contrib/metrics/test_precision_recall_curve.py Show resolved Hide resolved

sayantan1410 force-pushed the tests branch from 0df5e16 to 76fd0f7 Compare March 18, 2022 17:33

sayantan1410 and others added 2 commits March 20, 2022 18:44

added manual_seed to _test function

42f5660

autopep8 fix

89d14d8

Merge branch 'master' into tests

9285ca5

vfdev-5 reviewed Mar 20, 2022

View reviewed changes

fixed tolerance of pytest.approx

de96850

vfdev-5 reviewed Mar 21, 2022

View reviewed changes

sayantan1410 changed the title ~~Fixed a test in test_precision_recall_curve.py~~ Fixed test in test_precision_recall_curve and tests/ignite/contrib/metrics/regression Mar 21, 2022

vfdev-5 reviewed Mar 21, 2022

View reviewed changes

Added torch.manual_seed inside _test function

d578dde

reduced tolerance to default

a05cda1

vfdev-5 closed this Sep 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed test in test_precision_recall_curve and tests/ignite/contrib/metrics/regression #2511

Fixed test in test_precision_recall_curve and tests/ignite/contrib/metrics/regression #2511

sayantan1410 commented Mar 11, 2022

vfdev-5 commented Mar 11, 2022

sdesrozis commented Mar 11, 2022

sayantan1410 commented Mar 12, 2022

sayantan1410 commented Mar 12, 2022

sdesrozis commented Mar 12, 2022

sayantan1410 commented Mar 12, 2022

sayantan1410 commented Mar 13, 2022 •

edited by vfdev-5

Loading

sayantan1410 commented Mar 13, 2022

vfdev-5 commented Mar 14, 2022

sayantan1410 commented Mar 14, 2022 •

edited

Loading

sayantan1410 commented Mar 20, 2022

vfdev-5 Mar 20, 2022

sayantan1410 Mar 20, 2022

vfdev-5 Mar 21, 2022

sayantan1410 Mar 21, 2022

vfdev-5 Mar 21, 2022

sayantan1410 Mar 22, 2022

sayantan1410 commented Mar 20, 2022

sayantan1410 commented Mar 21, 2022

vfdev-5 commented Mar 21, 2022

sayantan1410 commented Mar 21, 2022

vfdev-5 commented Mar 21, 2022

vfdev-5 Mar 21, 2022

sayantan1410 Mar 21, 2022

vfdev-5 Mar 21, 2022

vfdev-5 Mar 21, 2022

sayantan1410 Mar 21, 2022

sayantan1410 commented Mar 23, 2022

vfdev-5 commented Apr 9, 2022

sayantan1410 commented Apr 9, 2022

vfdev-5 commented Sep 4, 2022

Fixed test in test_precision_recall_curve and tests/ignite/contrib/metrics/regression #2511

Fixed test in test_precision_recall_curve and tests/ignite/contrib/metrics/regression #2511

Conversation

sayantan1410 commented Mar 11, 2022

vfdev-5 commented Mar 11, 2022

sdesrozis commented Mar 11, 2022

sayantan1410 commented Mar 12, 2022

sayantan1410 commented Mar 12, 2022

sdesrozis commented Mar 12, 2022

sayantan1410 commented Mar 12, 2022

sayantan1410 commented Mar 13, 2022 • edited by vfdev-5 Loading

sayantan1410 commented Mar 13, 2022

vfdev-5 commented Mar 14, 2022

sayantan1410 commented Mar 14, 2022 • edited Loading

sayantan1410 commented Mar 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayantan1410 commented Mar 20, 2022

sayantan1410 commented Mar 21, 2022

vfdev-5 commented Mar 21, 2022

sayantan1410 commented Mar 21, 2022

vfdev-5 commented Mar 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayantan1410 commented Mar 23, 2022

vfdev-5 commented Apr 9, 2022

sayantan1410 commented Apr 9, 2022

vfdev-5 commented Sep 4, 2022

sayantan1410 commented Mar 13, 2022 •

edited by vfdev-5

Loading

sayantan1410 commented Mar 14, 2022 •

edited

Loading