Fixing issue 120 moving to cpu output of softmax neigbor finder#121
Fixing issue 120 moving to cpu output of softmax neigbor finder#121
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #120 by addressing a device mismatch error that occurred during validation in the Deep Functional Maps training pipeline. The primary fix ensures that the SoftmaxNeighborFinder returns indices on CPU to prevent runtime errors when indices are used with CPU-based tensors in the GeodesicError loss computation.
Key changes:
- Fixed device mismatch in
SoftmaxNeighborFinderby moving output indices to CPU - Updated demo notebook with improved explanations and reorganized imports
- Updated geomstats dependency to use the main branch from git
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| geomfum/convert.py | Modified SoftmaxNeighborFinder.forward() to explicitly move indices to CPU device, fixing the RuntimeError in issue #120 |
| notebooks/demos/Deep-Functional-Maps-Demo.ipynb | Reorganized imports, added explanatory cell for RobustFMNet usage, changed dataset paths from smal to faust, and adjusted k parameter from 200 to 30 |
| pyproject.toml | Updated geomstats dependency to use git repository main branch instead of a released version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "# we cna also use more robust models as\n", |
There was a problem hiding this comment.
Spelling error: "cna" should be "can".
| "# we cna also use more robust models as\n", | |
| "# we can also use more robust models as\n", |
| "# we cna also use more robust models as\n", | ||
| "# functional_map_model = RobustFMNet(\n", | ||
| "# feature_extractor=feature_extractor, fmap_module=fmap_module, converter=P2pFromFmConverter(neighbor_finder=SoftmaxNeighborFinder()),\n", | ||
| "# )\n" |
There was a problem hiding this comment.
The commented example code references P2pFromFmConverter and SoftmaxNeighborFinder, but these imports were removed from the import cell. If users want to use this example, they would need to add these imports back. Consider either: (1) keeping these imports in the first cell to support this example, or (2) adding a comment in this cell noting that the imports need to be added if this code is uncommented.
| "# )\n" | |
| "# )\n", | |
| "# Note: if you uncomment the example above, you must also import RobustFMNet, P2pFromFmConverter, and SoftmaxNeighborFinder in the imports cell.\n" |
| test-scripts = ["nbformat", "nbconvert", "ipykernel", "ipython", "pyvista", "plotly"] | ||
| test = [ | ||
| "pytest", | ||
| "geomstats@git+https://github.com/geomstats/geomstats.git@main", |
There was a problem hiding this comment.
The geomstats test dependency is now pulled from git+https://github.com/geomstats/geomstats.git@main, which means every test run fetches and executes arbitrary code from the mutable main branch of a third-party repository. If your CI or developer environments run tests with access to credentials or build artifacts, a compromise of that repository (or its branch) could be used to execute malicious code in your pipeline; prefer pinning to an immutable commit SHA or a vetted release instead of a moving branch reference.
gviga
dismissed
GiLonga’s stale review
The merge-base changed after approval.
This Pr fix the bug related to SoftmaxNeighborFinder, moving the output of the finder to 'cpu'.
Moreover, slightly improves the DeepFunctionalMaps notebook, providing more explanation on when to use the SoftmaxNeighborFinder or a standard Finder.
This Fixes #120 .