Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SignalFillEmptyd tranform #7011

Merged
merged 8 commits into from
Sep 21, 2023
Merged

Conversation

matt3o
Copy link
Contributor

@matt3o matt3o commented Sep 19, 2023

As mentioned here #2637 (comment) , this transform helps to fix NaN values after the input transforms.
It is basically only a wrapper around SignalFillEmpty.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Matthias Hadlich <matthiashadlich@posteo.de>
Signed-off-by: Matthias Hadlich <matthiashadlich@posteo.de>
Copy link
Contributor

@wyli wyli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, please add a unit test following the existing one https://github.com/Project-MONAI/MONAI/blob/dev/tests/test_signal_fillempty.py

not sure if replacement should allow for different inputs for different items in the keys, I guess we can expand the logic later if needed.

@matt3o
Copy link
Contributor Author

matt3o commented Sep 20, 2023

@wyli Sweet, thanks! I'll go add the tests and add it to the symbol table.

Also I realized the array test is broken: https://github.com/Project-MONAI/MONAI/blob/dev/tests/test_signal_fillempty.py#L27C1-L36C1

self.assertTrue(not np.isnan(fillemptysignal.any())) always evaluates to True if there is any non-zero value in the array. Can I fix that in this commit as well or do you want a separate one?
The fix is simple self.assertTrue(not np.isnan(fillemptysignal).any())

Signed-off-by: Matthias Hadlich <matthiashadlich@posteo.de>
Signed-off-by: Matthias Hadlich <matthiashadlich@posteo.de>
Signed-off-by: Matthias Hadlich <matthiashadlich@posteo.de>
Signed-off-by: Matthias Hadlich <matthiashadlich@posteo.de>
@wyli
Copy link
Contributor

wyli commented Sep 20, 2023

thanks @matt3o please include the test case fix in this PR..

matt3o and others added 2 commits September 21, 2023 19:15
Signed-off-by: Matthias Hadlich <matthiashadlich@posteo.de>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@wyli
Copy link
Contributor

wyli commented Sep 21, 2023

/build

@wyli wyli marked this pull request as ready for review September 21, 2023 18:50
@wyli wyli merged commit def73cf into Project-MONAI:dev Sep 21, 2023
30 of 35 checks passed
@matt3o matt3o deleted the SignalFillEmptyd branch September 22, 2023 12:14
wyli pushed a commit that referenced this pull request Oct 5, 2023
Related to #7011. I was using the `Signalfillemptyd` (based on
`Signalfillempty`) transform in monailabel and found out, it currently
allows no inversion.
When digging deeper I realized that SignalFillEmpty just throws away the
meta information.
With the simple addition of `track_meta=True` it works as expected.
I hope it has no other impact, but I honestly don't know.

@wyli would be cool if we can add this to MONAI 1.3.0, thanks! I can
also rework `Signalfillempty` to just accept any datatype, if that would
the more appropriate approach.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.

Signed-off-by: Matthias Hadlich <matthiashadlich@posteo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants