-
Notifications
You must be signed in to change notification settings - Fork 13
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
514 add channel masking #554
Conversation
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 left my comments below. Thanks for your effort @cwmeijer . I think we can have a discussion about it. I have some questions regarding the implementation. Just need to clarify a few things. Thanks in advance.
return np.concatenate([time_step_masks, channel_masks, number_of_combined_masks], axis=0) | ||
|
||
|
||
def generate_channel_masks(input_data: np.ndarray, number_of_masks: int, p_keep: float): |
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.
This is the duplication part you mentioned in the top post I think, just a reminder for ourselves, that it will be solved in PR #562, I will leave a comment there as well.
number_of_channel_masks = number_of_masks // 3 | ||
number_of_time_step_masks = number_of_channel_masks | ||
number_of_combined_masks = number_of_masks - number_of_time_step_masks - number_of_channel_masks |
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.
This is quite an interesting way to implement the masking for multi-channels. I can understand that by doing this we can have a very balanced masking array, which contains masking for entire channels, masking for certain time steps across channels and a mixture of them. But I have a feeling that this makes it a bit too complex.
What I have in mind are two simple ways:
- Simple flatten the input data and mask them brute-forcely
- Loop through all channels and treat them individually (mask each channel separately and concatenate them)
I can imagine that the current implementation makes the segmentation very tricky to code. Let's have a chat about it. It could be possible that I misunderstand something.
But thanks a lot for the effort! This also provides more insight about what we want.
assert np.any(result) | ||
assert np.any(~result) |
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.
Smart check!
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.
Thanks for the explanation @cwmeijer. To clarify a bit, this implementation tends to mask the timestep across all channels, a complete channel and a mixing of these two. This way we can have it more structured and it also allows us to implement the same strategy as we have for image masking based on wave function.
I checked the code and all the tests. They look very good to me. Nice work @cwmeijer! I think we can merge this now and continue working on the segmentation.
But since the algorithm is not so straight forward, we need to add more documentation/docstrings/examples to explain it. I will create a new issue for this and we also add this for the smart masking strategy based on wave functions for the images, which was not added before.
The purely random masking is a low-hanging fruit which can be another option to this. We can implement it later when working on Lime implementation for timeseries. I will create another issue for that.
Overall, nice work and thanks for all the strategical and smart thinking behind all the codes @cwmeijer 👍 .
Adds the ability to channel certain channels.
We can now
We cannot do any completely random masking, which is fine I think, but could be discussed.
Note that I left some code duplication in the code for these different masking operations. I will change one of the 2 duplicates when I implement #546.