Fix #470 non adapted sample indices in gradients during sorting #480
Conversation
|
Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping |
python/metatensor-operations/metatensor/operations/_dispatch.py
Outdated
Show resolved
Hide resolved
agoscinski
force-pushed
the
sort-gradfix
branch
3 times, most recently
from
7dbd0ed
to
a553a3d
Compare
| descending: bool, | ||
| ) -> TensorBlock: | ||
| """ | ||
| Sorts a single TensorBlock without the user input checking and sorting of gradients |
There was a problem hiding this comment.
| Sorts a single TensorBlock without the user input checking and sorting of gradients | |
| Sorts a single gradient TensorBlock. This is different from `_sort_single_block` because we need to update the sample differently (since gradient samples are pointers into the values samples) |
There was a problem hiding this comment.
updated this doc and embedded suggested change into it
| properties=Labels(["p"], np.array([[1], [0]])), | ||
| ) | ||
|
|
||
| # fmt: off |
There was a problem hiding this comment.
not needed but black makes this really hard to read so I turned it off
There was a problem hiding this comment.
I partially agree with you that black is not ideal here, but I would prefer not to start adding fmt: off or # noqa too often in the code. I would rather live with slightly worst formatting but have it consistent everywhere.
There was a problem hiding this comment.
This have strong feelings on. This sort test is already hard to understand as it is, and the black formatting made it much harder.
There was a problem hiding this comment.
Unlike other tests, the order must be much easier to read so one can follow what is how moved. I added comments why it is turned off.
|
Added commit with changes from review suggestions |
agoscinski
force-pushed
the
sort-gradfix
branch
from
1cca318
to
f495f7e
Compare
agoscinski
force-pushed
the
sort-gradfix
branch
2 times, most recently
from
e716901
to
55c994e
Compare
- Splitting sorted tensors into an ascending and descending so the fix can be more robustly tested - Adding dispatched arange_like to create inverse mapping for reasigning samples in gradients
agoscinski
force-pushed
the
sort-gradfix
branch
from
55c994e
to
08e19e7
Compare
Fixes #470
Waiting till #479 is merged to also add finite difference test
Contributor (creator of pull-request) checklist
Reviewer checklist
📚 Documentation preview 📚: https://metatensor--480.org.readthedocs.build/en/480/