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

514 add channel masking #554

Merged
merged 6 commits into from
Apr 25, 2023
Merged

514 add channel masking #554

merged 6 commits into from
Apr 25, 2023

Conversation

cwmeijer
Copy link
Contributor

@cwmeijer cwmeijer commented Apr 13, 2023

Adds the ability to channel certain channels.

We can now

  • mask channels
  • mask time steps
  • make a combination of the above

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.

@cwmeijer cwmeijer changed the title add channel masking (refs #514) 514 add channel masking Apr 13, 2023
@cwmeijer cwmeijer marked this pull request as ready for review April 13, 2023 14:01
@cwmeijer cwmeijer requested a review from geek-yang April 18, 2023 08:51
Copy link
Member

@geek-yang geek-yang left a 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):
Copy link
Member

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.

Comment on lines +20 to +22
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
Copy link
Member

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:

  1. Simple flatten the input data and mask them brute-forcely
  2. 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.

Comment on lines +129 to +130
assert np.any(result)
assert np.any(~result)
Copy link
Member

Choose a reason for hiding this comment

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

Smart check!

Copy link
Member

@geek-yang geek-yang left a 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 👍 .

@cwmeijer cwmeijer merged commit 3f97c30 into main Apr 25, 2023
@cwmeijer cwmeijer deleted the 514-channel-masking branch April 25, 2023 09:47
@geek-yang geek-yang linked an issue Apr 25, 2023 that may be closed by this pull request
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.

add channel masking
2 participants