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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add torch metrics support #1195

Closed
10 tasks done
bagxi opened this issue Apr 23, 2021 · 9 comments 路 Fixed by #1214
Closed
10 tasks done

Add torch metrics support #1195

bagxi opened this issue Apr 23, 2021 · 9 comments 路 Fixed by #1214
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@bagxi
Copy link
Member

bagxi commented Apr 23, 2021

馃悰 Bug Report

AdditiveValueMetric assumes that the metric value can be converted to float / NumPy array. But in reality, there are many cases when it is much faster and easier to calculate metrics on GPU using PyTorch. And this will cause errors as torch.Tensor cannot be converted to NumPy automatically.

How To Reproduce

Steps to reproduce the behavior:

  1. Use FunctionalMetricCallback with any metric function that returns torch.Tensor e.g. piq.psnr

Code sample

https://github.com/leverxgroup/esrgan/blob/update-catalyst/esrgan/callbacks/metrics.py#L20

Expected behavior

It would be great if utils.detach_tensor was used before metrics averaging.

Environment

Please copy and paste the output from our environment collection script

catalyst-contrib --collect-env
# or manually
wget https://raw.githubusercontent.com/catalyst-team/catalyst/master/catalyst/contrib/scripts/collect_env.py
python collect_env.py

(or fill out the checklist below manually).

# example checklist, fill with your info
Catalyst version: 20.04.2
PyTorch version: 1.8.0
TensorFlow version: N/A
TensorBoard version: 2.2.1
OS: Ubuntu 20.04 LTS
Python version: 3.8
CUDA runtime version: No CUDA
Nvidia driver version: No CUDA
cuDNN version: No CUDA

Additional context

Checklist

  • bug description
  • steps to reproduce
  • expected behavior
  • environment
  • code sample / screenshots

FAQ

Please review the FAQ before submitting an issue:

@bagxi bagxi added bug Something isn't working help wanted Extra attention is needed labels Apr 23, 2021
@Scitator
Copy link
Member

Nice catch!
Do you think adding mode=["numpy", "torch"] would be a good idea for AdditiveValueMetric?

@Scitator Scitator added the good first issue Good for newcomers label Apr 29, 2021
@bagxi
Copy link
Member Author

bagxi commented May 4, 2021

I think it will be easier to check types of inputs and use utils.detach_tensor in case of torch tensors?

@Scitator
Copy link
Member

Scitator commented May 5, 2021

why you could not specify that during init?
checking dtype each batch looks like performance killer

@bagxi
Copy link
Member Author

bagxi commented May 5, 2021

Sometimes people may have no ideas about types. And it also will be much easier to use this thing.

Can we assume that dtype of each batch is the same? If yes, then we can check dtype only for the first batch and store this information inside AdditiveValueMetric.

@Scitator
Copy link
Member

Scitator commented May 5, 2021

the problem here is that it requires you to check the type each batch, which is some kind of slow
moreover... how could we do not know the dtype of the returned metric value? it should be documented, I hope

@bagxi
Copy link
Member Author

bagxi commented May 6, 2021

And what about the function that will redefine itself on the first batch? In this case, dtype will be checked only once, but it is a little bit tricky approach.

@Scitator
Copy link
Member

Scitator commented May 6, 2021

馃 do you have an example of such a function?

@bagxi
Copy link
Member Author

bagxi commented May 7, 2021

class Metric:
    # ...

    def update(self, value, ...):
        self.value = self.compute_wrapper(value)
        # ...

    def compute_wrapper(self, value):
        if torch.is_tensor(value):
            self.compute_wrapper = catalyst.utils.detach_tensor
        else:
            self.compute_wrapper = lambda x: x

    def compute(self):
        # ...
        return value

@Scitator
Copy link
Member

Scitator commented May 7, 2021

why should we use such a metric with undefined behavior? do you have any real-metric examples?

@bagxi bagxi linked a pull request May 16, 2021 that will close this issue
20 tasks
@bagxi bagxi removed a link to a pull request May 16, 2021
20 tasks
@bagxi bagxi linked a pull request May 16, 2021 that will close this issue
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants