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

set Image dtype to numeric #369

Merged
merged 4 commits into from Jan 3, 2020
Merged

set Image dtype to numeric #369

merged 4 commits into from Jan 3, 2020

Conversation

bendichter
Copy link
Contributor

@bendichter bendichter commented Jan 2, 2020

set Image dtype to numeric
fix #368

set Image dtype to numeric
set dtype to numeric for Image subtypes
@bendichter bendichter changed the title fix #368 set Image dtype to numeric Jan 2, 2020
oruebel
oruebel previously approved these changes Jan 2, 2020
@rly
Copy link
Contributor

rly commented Jan 2, 2020

@bendichter as an aside, is it necessary for subclasses of Image to also define dtype? Shouldn't they inherit dtype from Image? Or are you just being explicit about the dtype?

@bendichter
Copy link
Contributor Author

@rly How exactly we want to treat dtype is still under discussion here. I realized recently that dtype was defined by the schema language as a required key, but is often omitted. Ttrying to add dtype will cause problems for non-specific classes like VectorData that could be numeric or string, so we decided to make it optional specifically in those types of cases. You are right that for specific classes we could have dtype be inherited from a parent, but I thought we might as well be explicit if we can and stay as close as we can to the original intent of the schema language. Do you think it would be better to remove dtypes that could be inherited? I'd be open to that.

@bendichter bendichter merged commit 34152b1 into dev Jan 3, 2020
@rly rly deleted the fix/dtype_image branch January 3, 2020 00:40
@rly
Copy link
Contributor

rly commented Jan 3, 2020

@bendichter I assume the dtype is automatically inherited by subclasses if it is not specified. Is that not the case?

I am fine with both being explicit about the dtype in a subclass and omitting an inherited dtype to remove redundancy and simplify the schema, though in general, I have a slight preference for the latter. So, this is good with me.

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.

Specify dtype for Image and GrayscaleImage
3 participants