Add new metric `d_lambda` #855

ankitaS11 · 2022-02-22T18:23:11Z

What does this PR do?

Adds New Image Metric - D_Lambda (Spectral Distortion Index)

Part of #799

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure to update the docs?
Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

for more information, see https://pre-commit.ci

…rics into feature/799_DLambda_metric

ankitaS11 · 2022-02-22T18:28:43Z

Note to the reviewers: Working on adding tests and updating doc examples. I will make this PR ready for review very soon.

ankitaS11 · 2022-02-25T10:53:18Z

Hi @stancld @SkafteNicki, I have finished implementing d_lambda, but I need help with testing this because -

UQI is used in d_lamba and sewar uses uniform filter in uqi while we use gaussian filter (the paper mentions gaussian distribution).
Couldn't find any reference implementation for d_lambda which matches the formula in paper.

If you can please review the code once and suggest testing strategy, that would be a great help.

for more information, see https://pre-commit.ci

…rics into feature/799_DLambda_metric

SkafteNicki

For testing, what about comparing to the implementation in https://github.com/andrewekhalel/sewar?

Also remember:

Remember to add entry to Changelog.md.
Remember to add reference in the docs

torchmetrics/image/d_lambda.py

torchmetrics/functional/image/d_lambda.py

ankitaS11 · 2022-02-25T14:17:32Z

For testing, what about comparing to the implementation in https://github.com/andrewekhalel/sewar?

Also remember:

Remember to add entry to Changelog.md.

Remember to add reference in the docs

Thank you for the review @SkafteNicki :)
sewar uses uniform filter in uqi and we have used gaussian filter (as the reference research paper mentions gaussian distribution). So I think we cannot compare our implementation with sewar.

Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>

for more information, see https://pre-commit.ci

SkafteNicki · 2022-02-25T15:13:20Z

@ankitaS11 in that case, we only have the option to write have a similar implementation here:
https://github.com/PyTorchLightning/metrics/blob/master/tests/helpers/non_sklearn_metrics.py
written in numpy which we can compare against. It is not optimal, but that is just how it is with some of the newer metrics proposed in research papers.

stancld · 2022-02-25T15:18:17Z

@ankitaS11 in that case, we only have the option to write have a similar implementation here: https://github.com/PyTorchLightning/metrics/blob/master/tests/helpers/non_sklearn_metrics.py written in numpy which we can compare against. It is not optimal, but that is just how it is with some of the newer metrics proposed in research papers.

Or maybe we can adjust the implementation of UQI so that both Gaussian and uniform kernel can be used?

codecov · 2022-03-01T12:52:55Z

Codecov Report

Merging #855 (b3e466b) into master (3aba057) will increase coverage by 0%.
The diff coverage is 98%.

@@          Coverage Diff          @@
##           master   #855   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files         167    169    +2     
  Lines        6960   7024   +64     
=====================================
+ Hits         6601   6664   +63     
- Misses        359    360    +1

SkafteNicki · 2022-03-01T13:26:54Z

Hi @ankitaS11,
Core part of the code looks good to me. I added some remaining things, trying to get it ready for merging.
Thanks :]

stancld

LGTM! :]

cc: @SkafteNicki @justusschock Just one thing - don't we wanna use preds, target naming instead of ms, fused and just reference to these naming in the docstrings?

justusschock · 2022-03-01T13:52:55Z

@stancld , right. Totally missed that. I think we should do that for consistency :)

tests/helpers/non_sklearn_metrics.py

stancld · 2022-03-01T15:19:40Z

torchmetrics/functional/image/d_lambda.py

+ ms: Tensor,
+ fused: Tensor,


Suggested change

ms: Tensor,

fused: Tensor,

preds: Tensor,

target: Tensor,

@ankitaS11 Please use preds, target naming for variables to stay consistent with the rest of torchmetrics package :]

I've a question regarding this, @stancld - since this metric (D_LAMBDA) is a no-reference image metric, so we don't have a target per say... Do you still think that renaming it to preds/target is a good idea?

I believe it's a little confusing, but I also agree that we should stay consistent. So I don't have a strong opinion here, and if you say, I'm happy to rename it for now. Maybe, in the future, we can think of more generic names for all the metrics in the torchmetrics API. Just sharing my thoughts :)

cc: @SkafteNicki @justusschock

@ankitaS11 I would lean towards having preds, target for the function call with some more detailed description referencing to ms, fused in the accompanied docstring :]

tests/helpers/non_sklearn_metrics.py

Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>

ankitaS11

Doc tests are failing, so can someone help with my comment?

ankitaS11 · 2022-03-02T08:23:20Z

torchmetrics/functional/image/d_lambda.py

+ >>> import torch
+ >>> _ = torch.manual_seed(42)
+ >>> ms = torch.rand([16, 3, 16, 16])
+ >>> fused = ms * 0.75
+ >>> ms, target = _d_lambda_update(ms, fused)
+ >>> _d_lambda_compute(ms, fused)
+ tensor(3.4769e-08)


Is there any way to skip the doc tests? In uqi similar test was passing as we had a constant factor of 0.75 for distortion. (and its a full reference metric so it will give same result without torch.manual_seed). Probably torch.manual_seed is not solving the problem here as the test is failing everytime with a different expected value.

lets use the manual_seed

@Borda, I just checked the CI, it seems to be failing for multiple environments with different values (Got: some random value) even after using manual_seed.

Eg: https://github.com/PyTorchLightning/metrics/runs/5382467199?check_suite_focus=true, https://github.com/PyTorchLightning/metrics/runs/5382472779?check_suite_focus=true

@ankitaS11 In the previous scenario, it seems like as ms and fused were too similar, the value of this metric was really close to 0 and there was a discrepancy on the 8th decimal place (first non-zero decimal) across some OS/torch versions. I change fused = ms * 0.75 to fused = torch.rand([16, 3, 16, 16]) so that those two tensors won't be too similar. Hopefully, it looks like everything works fine now :]

ankitaS11 · 2022-03-06T13:20:16Z

Hi, I accidentally messed up a few things in my fork, and deleted it :( Sorry, I will create a new one immediately, I hope that's okay.

ankitaS11 added 2 commits February 19, 2022 23:57

New Metric Added - D_lambda

72d12b7

New metric added - D_lambda

cb3b953

ankitaS11 requested review from Borda, SkafteNicki, justusschock, ananyahjha93, tchaton, ethanwharris and SeanNaren as code owners February 22, 2022 18:23

pre-commit-ci bot and others added 3 commits February 22, 2022 18:23

[pre-commit.ci] auto fixes from pre-commit.com hooks

670dc72

for more information, see https://pre-commit.ci

minor fix

5f0b1fe

Merge branch 'feature/799_DLambda_metric' of github.com:ankitaS11/met…

3b99f4d

…rics into feature/799_DLambda_metric

ankitaS11 marked this pull request as draft February 22, 2022 18:27

stancld mentioned this pull request Feb 25, 2022

Add CharacTER #633

Open

ankitaS11 and others added 4 commits February 25, 2022 16:26

minor fix

4e9160c

[pre-commit.ci] auto fixes from pre-commit.com hooks

a6c7a5a

for more information, see https://pre-commit.ci

minor fix

a7b1dbc

Merge branch 'feature/799_DLambda_metric' of github.com:ankitaS11/met…

e397e4a

…rics into feature/799_DLambda_metric

SkafteNicki reviewed Feb 25, 2022

View reviewed changes

ankitaS11 and others added 2 commits February 25, 2022 19:57

Apply suggestions from code review

f690732

Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

9d610f1

for more information, see https://pre-commit.ci

stancld and others added 4 commits February 26, 2022 13:08

Merge branch 'master' into feature/799_DLambda_metric

9ea3d58

Merge branch 'master' into feature/799_DLambda_metric

35f8b86

Added test cases

9d42667

resolving merge conflicts

72c21b1

update to new format

3fefb5f

SkafteNicki added this to the v0.8 milestone Mar 1, 2022

SkafteNicki added the New metric label Mar 1, 2022

fix doctests

d525c0b

SkafteNicki added 2 commits March 1, 2022 13:59

docs formatting

1ce2e25

typing

82ca554

SkafteNicki approved these changes Mar 1, 2022

View reviewed changes

justusschock approved these changes Mar 1, 2022

View reviewed changes

stancld approved these changes Mar 1, 2022

View reviewed changes

stancld reviewed Mar 1, 2022

View reviewed changes

tests/helpers/non_sklearn_metrics.py Outdated Show resolved Hide resolved

stancld reviewed Mar 1, 2022

View reviewed changes

ankitaS11 commented Mar 1, 2022

View reviewed changes

tests/helpers/non_sklearn_metrics.py Outdated Show resolved Hide resolved

ankitaS11 and others added 2 commits March 1, 2022 23:47

Apply suggestions from code review

3f309d6

Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>

Merge branch 'master' into feature/799_DLambda_metric

e666e0c

ankitaS11 commented Mar 2, 2022

View reviewed changes

mergify bot and others added 4 commits March 3, 2022 09:05

Merge branch 'master' into feature/799_DLambda_metric

d7a46ce

Merge branch 'master' into feature/799_DLambda_metric

eee44a7

Failing doctest: try to fix directly torch.rand generator

706d270

[Doctest] Generate fused randomly to avoid many decimal places

b3e466b

mergify bot added the ready label Mar 3, 2022

stancld mentioned this pull request Mar 4, 2022

More image metrics #799

Closed

ankitaS11 mentioned this pull request Mar 6, 2022

Adds new image metric - d_lambda #873

Merged

4 tasks

Borda added this to the v0.8 milestone May 5, 2022

Borda added the topic: Image label Aug 25, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new metric `d_lambda` #855

Add new metric `d_lambda` #855

ankitaS11 commented Feb 22, 2022 •

edited by SkafteNicki

Loading

ankitaS11 commented Feb 22, 2022

ankitaS11 commented Feb 25, 2022

SkafteNicki left a comment

ankitaS11 commented Feb 25, 2022

SkafteNicki commented Feb 25, 2022

stancld commented Feb 25, 2022

codecov bot commented Mar 1, 2022 •

edited

Loading

SkafteNicki commented Mar 1, 2022

stancld left a comment

justusschock commented Mar 1, 2022

stancld Mar 1, 2022

ankitaS11 Mar 1, 2022

stancld Mar 3, 2022

ankitaS11 left a comment

ankitaS11 Mar 2, 2022

Borda Mar 2, 2022

ankitaS11 Mar 2, 2022

stancld Mar 3, 2022 •

edited

Loading

ankitaS11 commented Mar 6, 2022

Add new metric d_lambda #855

Add new metric d_lambda #855

Conversation

ankitaS11 commented Feb 22, 2022 • edited by SkafteNicki Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

ankitaS11 commented Feb 22, 2022

ankitaS11 commented Feb 25, 2022

SkafteNicki left a comment

Choose a reason for hiding this comment

ankitaS11 commented Feb 25, 2022

SkafteNicki commented Feb 25, 2022

stancld commented Feb 25, 2022

codecov bot commented Mar 1, 2022 • edited Loading

Codecov Report

SkafteNicki commented Mar 1, 2022

stancld left a comment

Choose a reason for hiding this comment

justusschock commented Mar 1, 2022

stancld Mar 1, 2022

Choose a reason for hiding this comment

ankitaS11 Mar 1, 2022

Choose a reason for hiding this comment

stancld Mar 3, 2022

Choose a reason for hiding this comment

ankitaS11 left a comment

Choose a reason for hiding this comment

ankitaS11 Mar 2, 2022

Choose a reason for hiding this comment

Borda Mar 2, 2022

Choose a reason for hiding this comment

ankitaS11 Mar 2, 2022

Choose a reason for hiding this comment

stancld Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

ankitaS11 commented Mar 6, 2022

Add new metric `d_lambda` #855

Add new metric `d_lambda` #855

ankitaS11 commented Feb 22, 2022 •

edited by SkafteNicki

Loading

codecov bot commented Mar 1, 2022 •

edited

Loading

stancld Mar 3, 2022 •

edited

Loading