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

Adding the property_ranges query parameter and associated metadata #481

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

JPBergsma
Copy link
Contributor

@JPBergsma JPBergsma commented Jun 29, 2023

This PR describes how a client can request a specific part of a property that is returned as partial data.
I have tried to keep things as much as possible the same as in the original ranged properties proposal #452.

@JPBergsma JPBergsma marked this pull request as ready for review June 29, 2023 16:32
@blokhin blokhin changed the title Property_Ranges Querry Parameter Property_Ranges Query Parameter Jun 29, 2023
optimade.rst Outdated Show resolved Hide resolved
@rartino
Copy link
Contributor

rartino commented Jun 30, 2023

Thanks for writing this up!

Why did you move the whole section "Transmission of large property values"? It makes it tricky to see what has changed. Can you move it back?

Also, my interpretation of the workshop discussions was to make the property_ranges query parameter completely separate from, and orthogonal to, the mechanism for "Transmission of large property values" (this was a strong point of @sauliusg). Hence, it isn't clear to me why anything has to change in the "Transmission of large property values". Why can't we keep all documentation about this feature in the definition of the property_ranges query parameter?

@rartino rartino mentioned this pull request Jun 30, 2023
5 tasks
@JPBergsma
Copy link
Contributor Author

It seemed more logical to place the metadata section before the "Transmission of large property values" section, as the "Transmission of large property values" section depends on the metadata section but not the other way round. So it is more useful to read the metadata section before the "Transmission of large property values" section.

@JPBergsma
Copy link
Contributor Author

You are right, I could still separate the property ranges query parameter and the extra metadata fields from the transmission of large data section, as each could still be used separate from the other. I'll try to do this next week.

optimade.rst Outdated
@@ -567,6 +517,131 @@ Example of the corresponding metadata property definition contained in the field
}
// ...


Transmission of large property values
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify why I moved this section:

The section "Transmission of large property values" uses features from the section "Metadata properties". But the section "Metadata properties" does not depend on the section "Transmission of large property values" so I therefore placed the "Transmission of large property values" after the "Metadata properties" section.

I have made two small changes in the "Transmission of large property values" section:

Line 534 I replaced "one of alternative partial data formats " to "one of the partial data formats".
As I do not think the word alternative adds anything here.
Line 537: I broke the sentence into two parts and replaced "from the response, with three different links" with "from the response and includes three different links"

@ml-evs ml-evs added the blocking-release This is a PR or issue that presently blocks the release of next version of the spec. label Dec 19, 2023
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly formatting changes and suggesting rewordings -- I think I can go through and force most of them in if there are no objections

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@ml-evs ml-evs changed the title Property_Ranges Query Parameter Adding the property_ranges query parameter and associated metadata Dec 19, 2023
@rartino
Copy link
Contributor

rartino commented Jan 9, 2024

This PR appears a bit stalled and is important. @JPBergsma do you plan to continue working on this, or are you ok with me (or someone else) starting to merge changes into it?

@rartino rartino mentioned this pull request Jan 9, 2024
optimade.rst Outdated Show resolved Hide resolved
@rartino
Copy link
Contributor

rartino commented Jan 17, 2024

This PR makes a number of references to "range_ids", which is not the term used in the property definition PR #445. This needs to be adjusted.

@ml-evs ml-evs added this to the v1.2 milestone Jan 17, 2024
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
rartino and others added 2 commits March 22, 2024 02:07
Co-authored-by: Antanas Vaitkus <antanas.vaitkus90@gmail.com>
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated

- :field:`range_ids`: List of Strings.
A list with an identifier for each dimension of the property.
The outermost dimension of a nested array should come first.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The outermost dimension of a nested array should come first.
The outermost dimension of a nested array MUST come first.

I think making this a MUST would make it easier to use both for client and server.

optimade.rst Outdated
- :field:`range_ids`: List of Strings.
A list with an identifier for each dimension of the property.
The outermost dimension of a nested array should come first.
If, within one entry, dimensions for two or more properties share the same :field:`range_id` those dimensions should be thought of as the same dimension.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If, within one entry, dimensions for two or more properties share the same :field:`range_id` those dimensions should be thought of as the same dimension.
If, within one entry, dimensions for two or more properties share the same :field:`range_id` those dimensions should be treated as the same dimension.

optimade.rst Outdated
A list with an identifier for each dimension of the property.
The outermost dimension of a nested array should come first.
If, within one entry, dimensions for two or more properties share the same :field:`range_id` those dimensions should be thought of as the same dimension.
For example, if both the :property:`energy` and :property:`cartesian_site_positions` of a molecular dynamics trajectory share a range_id of :val:`frames`.
Copy link
Member

@merkys merkys Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For example, if both the :property:`energy` and :property:`cartesian_site_positions` of a molecular dynamics trajectory share a range_id of :val:`frames`.
For example, both the :property:`energy` and :property:`cartesian_site_positions` of a molecular dynamics trajectory may share a `:field:range_id` of :val:`frames`.

@ml-evs ml-evs added status/has-concrete-suggestion This issue has one or more concrete suggestions spelled out that can be brought up for consensus. PR/requires-discussion and removed blocking-release This is a PR or issue that presently blocks the release of next version of the spec. labels Mar 22, 2024
Co-authored-by: Andrius Merkys <andrius.merkys@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/requires-discussion status/has-concrete-suggestion This issue has one or more concrete suggestions spelled out that can be brought up for consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants