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

Add PearsonCorrelation metric #3212

Merged
merged 21 commits into from
Mar 30, 2024
Merged

Conversation

kzkadc
Copy link
Contributor

@kzkadc kzkadc commented Mar 20, 2024

Description: added Pearson correlation coefficient metric to ignite.contrib.metrics.regression.

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)

@github-actions github-actions bot added docs module: contrib Contrib module labels Mar 20, 2024
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 20, 2024

@kzkadc thanks for the PR, I haven't yet reviewed it in details, just a remark on the code location which could be better to be at ignite.metrics. We have a on-going work on moving contrib metrics to ignite.metrics and gradually removing the content of the contrib module keeping BC.

@kzkadc
Copy link
Contributor Author

kzkadc commented Mar 23, 2024

@vfdev-5 Is it better to create ignite.metrics.regression and place this metric there?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 23, 2024

@kzkadc yes, we'll move all ignite.contrib.metrics into ignite.metrics very soon, so I'd say let's just update your PR such that the metric is in ignite.metrics but we are using from ignite.contrib.metrics.regression._base import _BaseRegression temporarily.

@github-actions github-actions bot added the module: metrics Metrics module label Mar 24, 2024
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 29, 2024

Please also fix code formatting issue

https://github.com/pytorch/ignite/actions/runs/8469886156/job/23234585039?pr=3212

 + flake8 ignite tests examples --config setup.cfg
ignite/metrics/regression/pearson_correlation.py:12:121: E501 line too long (122 > 120 characters)
ignite/metrics/regression/pearson_correlation.py:15:121: E501 line too long (142 > 120 characters)
Error: Process completed with exit code 1.

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 a lot for working on this PR, @kzkadc !

Let's see if CI is globally passing the tests and it would be good to land it

@vfdev-5 vfdev-5 enabled auto-merge (squash) March 29, 2024 17:32
auto-merge was automatically disabled March 30, 2024 08:25

Head branch was pushed to by a user without write access

@vfdev-5 vfdev-5 merged commit 0dfeabe into pytorch:master Mar 30, 2024
16 of 20 checks passed
@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 30, 2024

@kzkadc can you please fix the issue with the implementation using non-cpu device:


self = <ignite.metrics.regression.pearson_correlation.PearsonCorrelation object at 0x7f2525d75150>
output = (tensor([0.9835, 0.2422, 0.7872, 0.0981, 0.6317, 0.2422, 0.2035, 0.0723, 0.5486,
        0.7612, 0.1874, 0.8356, 0.584...409e-03, 5.2258e-02, 7.6632e-01, 4.5562e-02,
        5.4043e-01, 4.6115e-01, 3.9585e-01, 5.3294e-01], device='cuda:0'))

    def _update(self, output: Tuple[torch.Tensor, torch.Tensor]) -> None:
        y_pred, y = output[0].detach(), output[1].detach()
>       self._sum_of_y_preds += y_pred.sum()
E       RuntimeError: Expected all tensors to be on the same device, but found at least two devices, cuda:0 and cpu!

https://github.com/pytorch/ignite/actions/runs/8490646759/job/23261938233#step:9:904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs module: contrib Contrib module module: metrics Metrics module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants