[FEAT] Add coarse-to-fine barycenter estimation #45

pbarbarant · 2024-04-17T14:03:32Z

Implements coarse-to-fine estimation of barycenters with sparse transport plans. Closes #52

Docstrings
Unit tests

… into feat_coarse_to_fine_bary

bthirion

LGTM, but the test could be more informative.

bthirion · 2024-04-22T21:29:51Z

tests/mappings/test_sparse_barycenter.py

+        fine_mapping_solver_params={"nits_bcd": 2, "nits_uot": 10},
+        device=device,
+        verbose=True,
+    )


This is only a smoke test ?

Yes this solely for the purpose of testing that everything runs regardless the validity of the transport plan.

Don't we need also a full test ?

I added new shape assertions + type checks

That's a good step forward.
But we may also want to test that the estimated barycenters are not crazy. Should we consider that in another PR for sparse and non-sparse baycenters altogether ?

Maybe we could test edge cases (although this may require using external libraries):

one could test that alpha=0 returns wasserstein barycenters similar to that of POT

similarly, one could test that alpha=1 returns gromov barycenters similar to that of POT

pbarbarant · 2024-05-02T15:06:56Z

Fixing tests once #48 is merged.

bthirion · 2024-05-02T19:37:44Z

Cna you take care of the failing tests ? LGTM otherwise.

bthirion

LGTM otherwise.

bthirion · 2024-05-06T21:13:25Z

tests/mappings/test_sparse_barycenter.py

+        fine_mapping_solver_params={"nits_bcd": 2, "nits_uot": 10},
+        device=device,
+        verbose=True,
+    )


Don't we need also a full test ?

…rameter

…r features

pbarbarant · 2024-05-22T16:51:56Z

New issue #52, sometimes features contain NaNs

bthirion

I like the homogeneous API !

The docstrings can still be improved a little bit...

bthirion · 2024-05-07T18:05:15Z

tests/mappings/test_sparse_barycenter.py

+        fine_mapping_solver_params={"nits_bcd": 2, "nits_uot": 10},
+        device=device,
+        verbose=True,
+    )


That's a good step forward.
But we may also want to test that the estimated barycenters are not crazy. Should we consider that in another PR for sparse and non-sparse baycenters altogether ?

src/fugw/mappings/sparse_barycenter.py

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

pbarbarant · 2024-05-23T21:28:51Z

I agree that future issues should be resolved in a new PR. I'm currently confronting the new features to real data which will bring its own set of bugs, but let's keep that separate.

bthirion · 2024-05-24T08:06:34Z

Agreed. Can you look at the failing test ?

pbarbarant · 2024-05-24T10:24:26Z

Agreed. Can you look at the failing test ?

All fixed!

bthirion

LGTM, thx.

bthirion · 2024-05-24T19:11:37Z

@alexisthual do you want to take a look ? Best,

alexisthual

LGTM!

Here are a few general comments that I think could be addressed in different PRs:

it would be nice to add docstrings to mappings/{barycenter,sparse_barycenter}.py (they would appear in the documentation)
it might never be useful, but the static method update_barycenter_geometry() is currently missing from FUGWSparseBarycenter (I'm totally fine with that, as we probably don't need to learn a matrix of anatomical distances between vertices at the moment -- I think it could be useful if someone were to compute FUGW barycenters using connectomes for anatomical distances for instance)

alexisthual · 2024-05-27T13:19:50Z

src/fugw/mappings/sparse_barycenter.py

+                source_sample=mesh_sample,
+                target_sample=mesh_sample,


I might be wrong, but I feel that you might need different values for mesh_sample across individuals if their data lie on different meshes. The barycenter might need its own mesh_sample as well?

You are definitely right. However we agreed with @bthirion to keep the same geometry/mesh as for now. Moreover I'm afraid of the large memory/speed slowdown that we could get if we individualize the geometry of each individual. Maybe a NotImplementedError would be the best fit ? I'll open an issue...

alexisthual · 2024-05-27T13:39:39Z

tests/conftest.py

+@pytest.fixture(scope="session", autouse=True)
+def ignore_sparse_csr_warning():
+    """Remove the warning for sparse CSR tensor support."""
+    warnings.filterwarnings(
+        "ignore", ".*Sparse CSR tensor support is in beta state.*"
+    )


I had never used this, nice trick! :)

alexisthual · 2024-05-27T14:02:21Z

src/fugw/scripts/coarse_to_fine.py

+    mask: torch.Tensor or None
+        Sparsity mask to use when fitting the fine mapping.
+        If None, a mask will be computed from the coarse mapping.


Feel free to ignore this comment, but would rather name this variable sparsity_mask so that it's not confused with a nilearn masker.

alexisthual · 2024-05-27T14:13:04Z

tests/mappings/test_sparse_barycenter.py

+        fine_mapping_solver_params={"nits_bcd": 2, "nits_uot": 10},
+        device=device,
+        verbose=True,
+    )


Maybe we could test edge cases (although this may require using external libraries):

one could test that alpha=0 returns wasserstein barycenters similar to that of POT

similarly, one could test that alpha=1 returns gromov barycenters similar to that of POT

pbarbarant added 6 commits April 17, 2024 12:28

Add sparse barycenter script

e4d16fe

Add FUGWSparseBarycenter class to mappings/__init__.py

c563c77

Update fit function in coarse_to_fine.py to include init_plan parameter

3695757

Remove callback in fit fn

853141f

Fix division error with sparse vectors in sparse barycenter

4fa8542

Add test for FUGWSparseBarycenter in test_sparse_barycenter.py

b137c47

pbarbarant added the enhancement New feature or request label Apr 17, 2024

pbarbarant marked this pull request as draft April 17, 2024 14:21

pbarbarant added 10 commits April 18, 2024 10:11

Add sparse barycenter script

b98eb10

Add FUGWSparseBarycenter class to mappings/__init__.py

1e32dc6

Update fit function in coarse_to_fine.py to include init_plan parameter

df47bd6

Remove callback in fit fn

23f23d2

Fix division error with sparse vectors in sparse barycenter

d854efd

Add test for FUGWSparseBarycenter in test_sparse_barycenter.py

1b05384

Merge branch 'feat_coarse_to_fine_bary' of github.com:pbarbarant/fugw…

952920e

… into feat_coarse_to_fine_bary

Update fit function in coarse_to_fine.py to include mask parameter

4f4d7dc

Update selection_radius parameter in FUGWSparseBarycenter class

6fc8006

Update fit function in coarse_to_fine.py to optimize mask computation

a07507a

bthirion reviewed Apr 22, 2024

View reviewed changes

pbarbarant added 3 commits May 2, 2024 14:41

Update selection_radius parameter in FUGWSparseBarycenter class

6ee4535

Update selection_radius parameter in FUGWSparseBarycenter class

1e30527

Update test_fugw_barycenter function in test_sparse_barycenter.py

b1e0af9

pbarbarant marked this pull request as ready for review May 2, 2024 15:11

pbarbarant added 5 commits May 6, 2024 10:32

Add sparse barycenter script

005a6d9

Add FUGWSparseBarycenter class to mappings/__init__.py

77a7d85

Update fit function in coarse_to_fine.py to include init_plan parameter

3c60c64

Remove callback in fit fn

513bfb5

Fix division error with sparse vectors in sparse barycenter

5257488

bthirion reviewed May 6, 2024

View reviewed changes

pbarbarant added 12 commits May 7, 2024 10:18

Update storing_device assignment in FUGWSparse class

2dfd979

Update fit function in coarse_to_fine.py to include storing_device pa…

0b6bf86

…rameter

Update test_fugw_barycenter function in test_sparse_barycenter.py

a67237a

Refactor FUGWSparseBarycenter class to remove unused variables

fd2d8d6

Update pytest configuration to ignore sparse CSR tensor warning

c346ef0

Update test_sparse_barycenter.py with assertions

ee557b3

Refactor test_sparse_barycenter.py with assertions and type checks

9297776

Refactor FUGWSparseBarycenter class to handle NaN values in fine plan

6015c8e

Add nan val check for barycenter features

038fec6

Add nan check in dense barycenters

4809deb

Refactor test_sparse_barycenter.py to include nan check for barycente…

f84926b

…r features

Refactor FUGWBarycenter class to remove unused variables

5249d8f

pbarbarant added 2 commits May 22, 2024 19:04

Add small reg to avoid zero-div

0929035

Add reg to dense barycenters to avoid NaNs

470822f

bthirion reviewed May 22, 2024

View reviewed changes

Fix docstring indentation

ac75b42

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

Fix docstrings and flake8

ef00241

bthirion approved these changes May 24, 2024

View reviewed changes

pbarbarant merged commit ce9e5f1 into alexisthual:main May 27, 2024
7 checks passed

pbarbarant deleted the feat_coarse_to_fine_bary branch May 27, 2024 14:00

alexisthual approved these changes May 27, 2024

View reviewed changes

This was referenced May 27, 2024

Rename sparsity_mask so that it's not confused with a nilearn masker. #56

Closed

[ENH] Add NotImplementedError for barycenters with different geometries #57

Closed

Test barycenter edge cases with POT #58

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Add coarse-to-fine barycenter estimation #45

[FEAT] Add coarse-to-fine barycenter estimation #45

pbarbarant commented Apr 17, 2024 •

edited

Loading

bthirion left a comment

bthirion Apr 22, 2024

pbarbarant Apr 24, 2024

bthirion May 6, 2024

pbarbarant May 7, 2024

bthirion May 7, 2024

alexisthual May 27, 2024

pbarbarant commented May 2, 2024

bthirion commented May 2, 2024

bthirion left a comment

bthirion May 6, 2024

pbarbarant commented May 22, 2024

bthirion left a comment

bthirion May 7, 2024

pbarbarant commented May 23, 2024

bthirion commented May 24, 2024

pbarbarant commented May 24, 2024

bthirion left a comment

bthirion commented May 24, 2024

alexisthual left a comment

alexisthual May 27, 2024

pbarbarant May 27, 2024

alexisthual May 27, 2024

alexisthual May 27, 2024

pbarbarant May 27, 2024

alexisthual May 27, 2024

[FEAT] Add coarse-to-fine barycenter estimation #45

[FEAT] Add coarse-to-fine barycenter estimation #45

Conversation

pbarbarant commented Apr 17, 2024 • edited Loading

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbarbarant commented May 2, 2024

bthirion commented May 2, 2024

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbarbarant commented May 22, 2024

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbarbarant commented May 23, 2024

bthirion commented May 24, 2024

pbarbarant commented May 24, 2024

bthirion left a comment

Choose a reason for hiding this comment

bthirion commented May 24, 2024

alexisthual left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbarbarant commented Apr 17, 2024 •

edited

Loading