Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 DensityFiltration #473
Add DensityFiltration #473
Changes from 6 commits
dfdad0f
7e7e8ab
48fe0ea
ee46506
a3bfc72
e2057cf
0df98e5
80f1ffa
9c72649
aa499a0
83210d8
9dd4fa5
5c9ea23
d1faa01
5937e64
8839b5d
8422a90
372b5b3
a67c47e
58c966d
f5002b0
6526bcf
7557715
4c2ff30
d94d057
87c4ef0
bbf3a45
716f246
2e72d01
359bb61
49344d0
44126c0
2b72331
f5cd329
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that, unlike some of the other transformers in
gtda.images
, this one will break on arrays which are not 3D. In your opinion, is it better to extend support to n-dimensional images, or turn this warning into an exception?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a tradeoff. I don't think I want to work on extending the transformers to images of arbitrary dimensions simply because I am not sure anyone you would use it.
At the same time, I won't deny them the possibility of trying to make it work. Let's fix this in October?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Nonetheless, why not throwing exceptions for now, instead of warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you throw exceptions, noone will be able to try passing 4d images, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but this transformer will break there (many
3
s hardcoded in the code), unless it is suitably generalized.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right! I changed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I misunderstood, but I still see a lot of
3
s in the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mask it computes is 3D, that's why images are reshaped in
transform
. I can then simply usenp.roll
in_calculate_density
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be forgotten leftover code here (maybe I was unclear in my review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not, you were not unclear, but you used a feature to change the code as suggestions and GitHub suggested me to commit them directly. That's what I did, but your suggestion snippets included only addition not change. xD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha! Thanks for pointing that out, I'll pay more attention to this (I suspect I failed to select multiple lines).