-
Notifications
You must be signed in to change notification settings - Fork 249
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
Update neo structure #1026
Update neo structure #1026
Conversation
Hello @JuliaSprenger! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-05-13 11:18:49 UTC |
The failing test is already fixed in #1025 |
9fb891d
to
ca3f4f3
Compare
Hi Julia. Thank you for updating the new diagram. About the hierachy place for RegionOfInterest I don't known what to say.
I let @apdavison check this new RegionOfInterest place in the tree. |
I think it would be good to discuss the missing links and pieces in the Neo structure rather sooner than later. @apdavison What is your opinion on the suggestions here? Do you have an alternative organization in mind? |
Here's a summary of the last discussion with @samuelgarcia @apdavison and @mdenker on this topic
|
- do not accept segments as child objects, but accept regionsofinterest - expand base class RegionOfInterest
- add ImageSequence and RegionOfInterest - remove outdated fake_neo reference - take into account if necessary attributes contain references to neo objects - shorted lines if too long for text box
|
||
|
||
class RegionOfInterest: | ||
class RegionOfInterest(Container): |
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 should be BaseNeo
from .basesignal import BaseSignal | ||
from .dataobject import ArrayDict | ||
|
||
|
||
class ChannelView(BaseNeo): |
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.
to be reverted or inherit from a BaseView
class
This PR is doing a lot of things. I think we should break it up into several PRs, then we can discuss each of them separately.
I'll follow up with PRs to:
|
@apdavison can we clos this in favor of #1313 and #1314 ? |
#1384 fixes the figure generation script, and is the final PR needed to replace this one. |
This PR is a suggestion on how to update the neo object linking to include RegionsOfInterest and ChannelView in a consistent manner.
Currently,
Group
s accept alsoSegment
s as child objects, which I think is confounding the neo structure. AlsoRegionOfInterest
was not considered as part of a neo structure yet. Since these are supposed to basically fulfill a masking functionality forImageSequence
their functionality is very similar to the one ofChannelView
s forSignal
s. So my suggestion is to haveGroup
andSegment
at the same level andRegionOfInterest
as well asChannelView
as optional level before linking to data object. This results in the following neo dependencies:This PR also updates the corresponding neo structure diagram (as well as generation script).
@samuelgarcia @apdavison What do you think of this?
Fixes #1024