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

BUG/MAINT: Have all metrics return tensors #701

Closed
1 task
NickleDave opened this issue Sep 23, 2023 · 1 comment
Closed
1 task

BUG/MAINT: Have all metrics return tensors #701

NickleDave opened this issue Sep 23, 2023 · 1 comment

Comments

@NickleDave
Copy link
Collaborator

The edit distance metrics in vak.metrics.distance currently return either Python ints or np.float64 scalars.

When models call the lightning method self.log with these values, there's an implicit conversion to a torch tensor with a dtype of float32--we currently get a warning about this during the validation step. In most cases this is good enough but it's at the very least kind of weird to store int counts of edits as floats.

What's more problematic is that this can cause a failure on some platforms, e.g. on an M1 Mac using mps as the accelarator, as described in #700--because the returned segment edit distance is np.float64, so converting to tensor gives a torch.float64, that the mps backend can't deal with. We should fix this

  • Have distance metrics return tensors (other metrics already do), that are either float32 or int32 as needed
NickleDave added a commit that referenced this issue Sep 23, 2023
Makes functions in `vak.transforms.distance.functional`
return tensors so we don't cause errors when lightning
tries to convert from numpy to tensors to log.

Letting lightning do the conversion kind of works,
but it can cause a fatal error
for someone using an Apple M1 with 'mps' as the accelerator,
see https://forum.vocalpy.org/t/vak-tweetynet-with-an-apple-m1-max/78/4?u=nicholdav

I don't find any explicit statement in either the Lightning
or Torchmetrics docs that metrics should always be tensors,
and that this guarantees there won't be weird issues
(right now we get a warning on start-up that all logged scalars
should be float32, but I would expect one should be able to log
integers too?).
But from various issues I read, it seems like that should be the case,
Lightning-AI/pytorch-lightning#2143
and I notice that torchmetrics classes tend to do things like
convert to a float tensor
NickleDave added a commit that referenced this issue Sep 23, 2023
Makes functions in `vak.transforms.distance.functional`
return tensors so we don't cause errors when lightning
tries to convert from numpy to tensors to log.

Letting lightning do the conversion kind of works,
but it can cause a fatal error
for someone using an Apple M1 with 'mps' as the accelerator,
see https://forum.vocalpy.org/t/vak-tweetynet-with-an-apple-m1-max/78/4?u=nicholdav

I don't find any explicit statement in either the Lightning
or Torchmetrics docs that metrics should always be tensors,
and that this guarantees there won't be weird issues
(right now we get a warning on start-up that all logged scalars
should be float32, but I would expect one should be able to log
integers too?).
But from various issues I read, it seems like that should be the case,
Lightning-AI/pytorch-lightning#2143
and I notice that torchmetrics classes tend to do things like
convert to a float tensor
@NickleDave
Copy link
Collaborator Author

Fixed by #702

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant