rusty1s / pytorch_scatter

PyTorch Extension Library of Optimized Scatter Operations
https://pytorch-scatter.readthedocs.io
MIT License
1.54k stars 179 forks source link

logsumexp handling of edge-cases #426

Open rasmushaugaard opened 6 months ago

rasmushaugaard commented 6 months ago

Hi! First of all thanks for this library, which has been useful in a few projects.

I've run into some inconsistencies between the torch_scatter.logsumexp and torch.logsumexp:

1) logsumexp of no elements should default to -inf, not 0. This is equivalent to having sum(..., start=0) in non-log space.
torch.logsumexp(torch.tensor([]), dim=0) returns -inf. 2) it should not hide nans: torch.logsumexp(torch.tensor([torch.nan]), dim=0) returns nan. 3) no eps should be added. torch.logsumexp also doesn't have this.

In the first commit, I've added test cases for empty lists, -inf, inf, very large positive and negative numbers - and added a cartesian product test across the inputs and the various floating point data types.

In the second commit, I've changed the implementation to comply with the new tests.

Dependency Management

On another note, I ended up spending a few hours setting up the torch/cuda dependencies correctly to build from source. I ended up using the following commands to install deps, build from source and run the tests:

cd pytorch_scatter
conda create -n torchscatter python=3.10 pytorch::pytorch pytorch::pytorch-cuda=11.8 nvidia/label/cuda-11.8.0::cuda
conda activate torchscatter
pip install -e ".[test]"
pytest

Maybe having this or something similar in the docs (if it's not already there and I missed it) could make it easier for others to contribute.

codecov-commenter commented 6 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.20%. Comparing base (140d3ad) to head (e0c09ae). Report is 7 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #426 +/- ## ========================================== + Coverage 97.23% 98.20% +0.97% ========================================== Files 10 10 Lines 217 223 +6 ========================================== + Hits 211 219 +8 + Misses 6 4 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rasmushaugaard commented 6 months ago

I just had a look at the history, and it seems an issue #368 motivated a change of how nans and infs were treated in #369 and introduced a new issue #407 - which is also the issue that motivated this pull request, which should solve both.

I've added tests and improved numerical stability for the inplace version.

github-actions[bot] commented 2 weeks ago

This pull request had no activity for 6 months. It will be closed in 2 weeks unless there is some new activity.

rasmushaugaard commented 2 weeks ago

Hi @rusty1s, Is there something I can do to help with this? :)