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
Adding the
property_ranges
query parameter and associated metadata #481base: develop
Are you sure you want to change the base?
Adding the
property_ranges
query parameter and associated metadata #481Changes from 34 commits
3b9cbc5
eae1679
f00b52d
2a0bef8
4a00af4
6905d7f
d579c6e
6792962
93ecd0b
a84c72e
dc11574
17beeef
cc89029
6d8e24f
680317b
f2a2799
63e3821
0efb5e7
bf7e5b4
d029756
63f122f
f863308
a73029a
c098e47
4f704c1
266545b
e562f49
315546a
ce9661b
358157f
7e29652
085030d
b3a96a0
6231d2a
9f639d1
46eafb5
657a696
919b83d
a623390
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.
In JSON? If we move to e.g. HDF5 then there the arrays will be a real distinct data type.
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 part discusses the data model internal to OPTIMADE itself, where we (for now) only formally need list in lists (but we say here ~"lets use the term 'arrays' for lists in lists"). How this data model is mapped onto JSON types is first described in section 4.2, where indeed lists -> JSON lists. A future section discussing HDF5, or for that matter XML, parquet, etc., would define how these types are mapped into native data types. It would be valid for HDF5 to say that lists in lists (in lists...) should be mapped to the HDF5 array format.
If you ask "why don't we just adopt arrays as a fundamental data type and say that in JSON arrays map to lists in lists", I think that would be a valid change (but not in this 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.
I would suggest using "list" instead of "array" everywhere, as in the specification "list" is defined as data type, whereas "array" is used much less often and mostly as a synonym of "list".
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.
You are right, good catch. Array should almost everywhere be replaced by list. Array should only be kept when we explicitly refer to JSON:API output arrays.
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.
After thinking about this, I don't find a good way to handle this without defining the term 'array' to be a general word for a list or a structure of nested lists. (IMO "Multidimensional lists" is far worse as a term.) So, I've pushed a commit that does this.
(Note: 'resolve' this conversation if you are happy with the fix, otherwise comment below).
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'll leave it up to somebody more experienced in the protocol to resolve 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 gather that "small" and "large" have a techncial meaning here (wrt to the transmission).
Do you have any kind of annotation trick to makes this clearer?
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.
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 like this better, yes. Given that only large has a meaning, it's better to remove small, as you did.
2 minor suggestions:
The protocol for large property values
: can this refer to the relevant section?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 realize that the way these comments chop up the text can make it a bit difficult to see the context, but the link you ask for is already right on the row before this line (numbered 616 right now).
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.
Above you used the term "array axis". I'd use the same term here, as with "index" idk whether you mean the dimension or a slice along a dimension.
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'm not sure what you mean here, can you use the GitHub suggest edit feature to show a suggestion? (Also, the discussion of terminology with regards to indices and axes may have been clarified by a discussion we had last web meet?)
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'd need a refresher there, sorry.
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.
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'm confused, this doesn't read cleanly to me in the context it appears. The first sentence in the suggestion isn't even a complete sentence?
The context here is that we are in the general text at the top of the section trying to outline what this feature is and how it is used. The sentence right now is just trying to communicate that the metadata property array_axis is one of the essential mechanisms of the protocol.
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, these are very different modes of querying. Typically, a client will have to query them in separate stages.
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'm not sure what you mean here: these fields are included as information from the server to the client to provide information related to slicing; so I'm not sure what the two different "modes" you refer to, or how you from what is written here or what stages you mean. Maybe you can clarify a bit?
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.
With 2 query modes, I meant:
Perhaps mode 1 could be split into 1.1 (sliceable metadata implicit requested) and 1.2 (sliceable metadata explicitly requested).
Anyhow, I was just asking for a clearer distinction between which query mode is covered where. Subtitles could work? Maybe I highlighted the wrong lines, but that would underscore my point.
Lastly, my 2nd question was about the responses when a query asks for both schema metadata and regular data.
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 one was kinda though to get on the first read. Maybe refer to the example below.
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.
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 think this would be clearer when presented as 2 bulletpoints in the format query: response.
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 think we need a suggested edit here to understand the format you are after.
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 I interpret this as a MUST? If so, I would put the stronger specifier before the weaker MAY.
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.
May the client request multiple (different) slices from the same array axis in the same query?
E.g.
property_slices=[dim_frames:3:37:5,dim_frames:49:105:2]
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 is clarified in "REQUIRED keys". A link suffices then.
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.
In the present version (which may have changed since you commented) I think this information is quite clear in the text right below.
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.
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's actually already explaiend that
:val:null
refers to the default value, and that it should be returned unless a specified value was used.From how I understood it, the same applies to the
start
andstep
keys too.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.
Is this then equivalent to
property_ranges=[<first_dim>:::, ...]
?I.e. should this return the full property or leave it out?
This might be defined somewhere else. If so, pls refer to that section.
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.
The query parameter not being included means the client isn't trying to use this protocol; so I think the natural interpretation is that "everything works as usual without the slices protocol". I guess that could be clarified somewhere, but I'm not sure exactly where.
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.
Minor style changes to make the phrase clearer:
dim_frames
anddim_sites
.the frames with indexes 0, 10, 20, 30, ..., 990
explains it enough. The paranteses else become too verbose.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 think this addresses @ndaelman-hu suggestions, except for removing the explanation in the last parenthesis, which from our discussions on the workshop I do think still is necessary to be completely clear.
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.
Her, you lost me
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.
@ndaelman-hu is this more clear?: