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

xarray parameters #404

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

xarray parameters #404

wants to merge 4 commits into from

Conversation

poplarShift
Copy link

Implements #381

I'm just putting this here for discussion. The PR would need some tests I imagine.

Questions:

  1. Should we validate the provided default? Currently, param is e.g. ok with param.Number(default=None, bounds=(1, 100)) but will throw an error at param.DataFrame(default=None, rows=(1, 10)).

  2. I've tried to carry over (from DataFrames) the distinction between passing sets or lists into the arguments. This might get confusing though... Opinions?

@poplarShift poplarShift changed the title Draft xarray parameters xarray parameters Apr 29, 2020
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Looks good to me! I haven't thought deeply about all the things one might want to validate on an Xarray dataset, and I haven't tried actually using it, but it looks like it's doing the right things to me.

param/__init__.py Show resolved Hide resolved
@jbednar
Copy link
Member

jbednar commented Apr 30, 2020

  1. Should we validate the provided default? Currently, param is e.g. ok with param.Number(default=None, bounds=(1, 100)) but will throw an error at param.DataFrame(default=None, rows=(1, 10)).

Param usually sets allow_None=True if the default is set to None, under the assumption that the Parameterized class author clearly thus intends None to be valid for this Parameter. If param.DataFrame does not do that, I would be happy for such code to be added to param.DataFrame (and to these new classes) to do that; it seems like a reasonable thing to do for the convenience of authors.

  1. I've tried to carry over (from DataFrames) the distinction between passing sets or lists into the arguments. This might get confusing though... Opinions?

I agree that's confusing, and completely did not remember that param.DataFrame acted that way. It's important to be able to make that distinction, and sets vs. list/dict does concisely convey that, but it's mysterious. It might be nice to have a more explicit way of conveying the same information, but I can't easily think of any non-clunky way to do it.

@poplarShift
Copy link
Author

poplarShift commented Apr 30, 2020

  1. I've tried to carry over (from DataFrames) the distinction between passing sets or lists into the arguments. This might get confusing though... Opinions?

I agree that's confusing, and completely did not remember that param.DataFrame acted that way. It's important to be able to make that distinction, and sets vs. list/dict does concisely convey that, but it's mysterious. It might be nice to have a more explicit way of conveying the same information, but I can't easily think of any non-clunky way to do it.

Then how about set is for minimal requirements, list is for an exact match, and tuple is not allowed? (For dataframes, tuples mean length constraints on the number of columns/rows, but of course constraining the number of dimensions or data variables on an xarray DataArray does not make much sense.)

I would argue that set is "marked" in the sense that using set is usually a conscientious choice whereas people don't pay much attention using lists or tuples. In that way, we would be making the exact matching the "default" behaviour (if you will), which I would argue is sensible.

A second option would be to introduce explicit boolean flags dims_exact etc. or something similar that convey the same information but I agree that's clunky.

@poplarShift
Copy link
Author

PS: Note that a list columns argument to DataFrame will also check for column order. I don't think that makes much sense for a DataArray, so I would drop that distinction here.

@jbednar
Copy link
Member

jbednar commented Apr 30, 2020

Then how about set is for minimal requirements, list is for an exact match, and tuple is not allowed?

Sounds reasonable to me.

@poplarShift poplarShift marked this pull request as draft May 12, 2020 13:48
@tonyfast tonyfast added this to the 2.0 milestone Aug 31, 2020
@philippjfr philippjfr marked this pull request as ready for review January 16, 2023 16:32
@maximlt
Copy link
Member

maximlt commented Apr 5, 2023

Just noting that this will need some tests and documentation before being merged.

@maximlt maximlt modified the milestones: 2.0, v2.x Apr 16, 2023
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.

5 participants