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 mask type control #119
Add mask type control #119
Changes from 10 commits
6215ef6
14226ce
7814c20
7eb0de6
7d3323a
e66f7f0
6797803
705ebd9
4daa4a4
800a1ce
717a8a0
048be06
5d5d704
dd4336e
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.
What do you think of the following logic. You simplify the conditions and conditional structure at the price of initializing plane segmentation in two different ways:
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.
Logic simplified to avoid additional cyclomatic complexity and mimic the switch statement that we'll eventually be able to use for this type of logic
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.
See other comment for how the column-wise was not working, but this is the only way I was able to successfully do this.
Note that the returned structure from ROIExtractors (the
pixel_masks
object) is anp.ndarray
of shape(num_rois, num_dimensions)
- attempting to add this without the list-compression tuple wrapping results in shape errors. The list-compression tuple wrapping was taken from the working example of the PyNWB documentation: https://pynwb.readthedocs.io/en/stable/tutorials/domain/ophys.html#pixel-masksThere 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.
Concerning the format, yeah, it looks strange but I remember having problems adding ragged data as column as well when working in the units table. That's why spikes over there are always added as rows.
You are already adding
id
above in line 663, as we don't have test for this part yet I don't know if this will cause troubles or does it just work as a reference here?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.
So that bit is important actually - if you know that the type of mask being written is one of the two types that currently has to be added by-row (since the column approach of #157 has yet to work), then you had to perform that row writing after the plane segmentation object is in memory but before adding other column data.
But also, when adding the data by row you have to set the ID there instead of as a separate column.
But if you're not writing any data row-wise, then you can pass the column of IDs directly into the initialization of the table as we have always done.
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.
Yep, same problem as with the other dynamic tables. I remember facing issues with this in the electrodes and units table that made the writing trickier. The problem as you mentioned is that id can't not be passed as a column itself. It needs to be passed at initialization or built row wise.
Strangely, I just tested this in a planet segmentation object intilized without ints and is asking for mask_type as a required argument of add_roi:
Which I find it strange that you can omit if you build everything by columns but you can't not build a table by rows without a type of mask.
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.
Might want to raise an issue in PyNWB for that lol
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.
Why is this and the lines below 1091-1023?
Should we remove them in another PR?
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.
Just linter cleanup, they're not doing anything. Technically the call to
get_module
might be initializing the processing module, but theophys
return is unused and the stuff below is unusedThere 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 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.
Can you add this to the changelog?
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.
Done