-
Notifications
You must be signed in to change notification settings - Fork 86
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 Image type and subtypes GrayScaleImage, RGBImage, RGBAImage #693
Conversation
* add Image to core.base * add GrayscaleImage, RGBImage, and RGBAImage to core.image * add pynwb support for Image, Images, GrayscaleImage, RGBImage, and RGBAImage TODO: add test
* make image subtypes groups instead of datasets * add doc fields to schema * add tests for all types
Codecov Report
@@ Coverage Diff @@
## dev #693 +/- ##
==========================================
+ Coverage 74.51% 74.57% +0.05%
==========================================
Files 58 58
Lines 6753 6776 +23
Branches 1393 1398 +5
==========================================
+ Hits 5032 5053 +21
- Misses 1328 1332 +4
+ Partials 393 391 -2
Continue to review full report at Codecov.
|
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.
Pending pass of the CI tests ;-)
I guess I was too soon with approving it. Looks like the CI fails. But other than the failed tests it looked good to me. |
thanks. I have addressed your comments
Do we want to enforce that |
Yeah that was my original plan but I didn't know how to get Images to work
with it
…On Tue, Oct 30, 2018, 4:14 PM Andrew Tritt ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/pynwb/data/nwb.base.yaml
<#693 (comment)>
:
> @@ -91,6 +91,24 @@ groups:
- neurodata_type_def: NWBDataInterface
neurodata_type_inc: NWBContainer
doc: An abstract data type for differentiating experimental data from metadata
+- neurodata_type_def: Image
Why is Image a group and not a dataset? Making a group requires the
additional dataset. This doubles the number of objects need to store an
image
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#693 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAziEhV_EvZLCEXDYvi74ar-j9LBW3rKks5uqLMRgaJpZM4YCgKw>
.
|
@bendichter I just submitted a PR to this branch. I thought seeing the diff would be easier than trying to explain it over text. |
* make Image basetype a dataset * move Image subtypes to datasets * fix dataset specs
@ajtritt can you review 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.
Some extra __nwbfields__, but other than that it looks good.
Motivation
fix NeurodataWithoutBorders/nwb-schema#143, fix NeurodataWithoutBorders/nwb-schema#129
Checklist
flake8
from the source directory.#XXX
notation whereXXX
is the issue number ?