pytorch_scatter icon indicating copy to clipboard operation
pytorch_scatter copied to clipboard

logsumexp handling of edge-cases

Open rasmushaugaard opened this issue 2 years ago • 5 comments

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.

rasmushaugaard avatar Mar 15 '24 09:03 rasmushaugaard

: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
@@            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.

codecov-commenter avatar Mar 15 '24 09:03 codecov-commenter

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.

rasmushaugaard avatar Mar 15 '24 12:03 rasmushaugaard

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

github-actions[bot] avatar Sep 12 '24 01:09 github-actions[bot]

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

rasmushaugaard avatar Sep 13 '24 21:09 rasmushaugaard