-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create specs #1
Create specs #1
Conversation
FYI I highly recommend using The 'code' for it can then live in the README or some other |
Not to mention the current extension is a lot more modular than most others - I'd recommend separate ER diagrams per related group; e.g., you could define a base |
Overall looks good - made first round of comments |
Yes, thank you for the suggestion. I was planning to do it in Mermaid once I finished this first step. I started with draw.io because it was a simple copy-paste from previous diagrams, and I just wanted to have a reference for this specific PR. |
README.md
Outdated
attributes | ||
-------------------------------------- | ||
microscopy_type : text, optional | ||
doi : text, optional |
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.
All microscopes have a DOI? I don't think I've ever seen that in the methods/supplemental section
How would I look it up for a particular microscope?
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.
It was meant to be there just for custom microscopes. I remember we discussed this a while ago, but I am also not totally convinced that this is something needed.
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.
Would a home-built microscope have a manufacturer
or model
number?
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.
probably not 😑, I need to change this 😅
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.
Where are we at on this? Does the base PyNWB device has the manufacturer
then you add the model
to the new BaseModel
?
Is this still a TODO? |
OpticalFilter *-- DeviceModel : extends | ||
BandOpticalFilter *-- OpticalFilter : extends | ||
EdgeOpticalFilter *-- OpticalFilter : extends | ||
DichroicMirror *-- DeviceModel : extends |
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.
Did you intend to duplicate the OpticalFilter and below in both the ophys and fiber photometry?
I think just including it in the separate block above is sufficient unless you also showed how those filters interact as metadata with the other extensions data series (which I think would be better in the respective extensions)
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! Agree I can simplify it like this
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 am correcting this in #2
1 question about simplifying the ER diagrams, otherwise looks good! |
TODO:
Entity relationship diagrams
Indicator and Effector
Optical Filters
Other devices in the optical setup