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

Extended xcube.core.store.DataStore docstring to include a basic conv… #331

Merged
merged 4 commits into from
Sep 30, 2020

Conversation

forman
Copy link
Member

@forman forman commented Sep 1, 2020

…ention for store open parameters.

Closes #330

@forman forman self-assigned this Sep 1, 2020
Copy link
Contributor

@TonioF TonioF left a comment

Choose a reason for hiding this comment

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

Data contraining open parameters would then be 'variable_names', 'bbox' and 'time_range' (other store-specific parameters may exist). As we have declared that these may be None, I think they should be set to optional in the list.

@forman
Copy link
Member Author

forman commented Sep 1, 2020

Data contraining open parameters would then be 'variable_names', 'bbox' and 'time_range' (other store-specific parameters may exist). As we have declared that these may be None, I think they should be set to optional in the list.

No, not necessarily. This is store dependent. Sentinel HUB for example does not define a useful default for bbox (whole world in 10m?) or spatial_res when we use a mixed set of Sentinel-2 bands.

EDIT

If we understand constraints in the sense of shaping the expected dataset, then the resolutions spatial_res and time_period are also constraining open parameters.

However, I will clarify that the rules describes are only valid for optional open parameters.

@forman forman requested a review from TonioF September 1, 2020 14:01
Copy link
Contributor

@TonioF TonioF left a comment

Choose a reason for hiding this comment

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

I approve, then.

Comment on lines 237 to 240
* _if P is None_ means, parameter not given, hence no constraint applies, hence full containment.
* _if not P_ means, we exclude what would otherwise be fully included.
* _else_, the given constraint applies.

Copy link
Member

Choose a reason for hiding this comment

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

I find this section a little unclear. I suggest:

  • Replace "full containment" with "no additional restrictions on requested data".
  • Replace "what would otherwise be fully included" with "data that would be included by default".

I'm also a bit concerned about if not P, which applies if P is None, False, 0, or any empty container. Is it really intended to apply so broadly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Is it really intended to apply so broadly?

Why not?


* ``variable_names: List[str]``: Included data variables.
Available coordinate variables will be auto-included for any dimension of the data variables.
* ``bbox: Tuple[float, float, float, float]``: Spatial coverage.
Copy link
Member

Choose a reason for hiding this comment

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

It would be useful to be explicit about the order of elements in bbox -- xmin, ymin, xmax, ymax, I assume.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

* ``bbox: Tuple[float, float, float, float]``: Spatial coverage.
* ``crs: str``: Spatial CRS, e.g. "EPSG:4326" or OGC CRS URI.
* ``spatial_res: float``: Spatial resolution in coordinates of the spatial CRS.
* ``time_range: Tuple[Optional[str], Optional[str]]``: Time range interval using iso-date/times.
Copy link
Member

Choose a reason for hiding this comment

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

We should be explicit about the semantics of None in the time range (which I assume should be interpreted as "from the earliest data in the dataset" and "up to the latest data in the dataset" for start and end respectively).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@forman
Copy link
Member Author

forman commented Sep 30, 2020

Data contraining open parameters would then be 'variable_names', 'bbox' and 'time_range' (other store-specific parameters may exist). As we have declared that these may be None, I think they should be set to optional in the list.

Right.

EDIT

No. It has been made clear, that all parameters are optional.

@forman forman merged commit 5467f1b into master Sep 30, 2020
@forman forman deleted the forman-330-store_conventions branch January 15, 2021 11:47
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.

Establish common data store conventions
4 participants