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

Hardiness zones #1396

Merged
merged 42 commits into from Jun 22, 2023
Merged

Hardiness zones #1396

merged 42 commits into from Jun 22, 2023

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Jun 16, 2023

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGES.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • Adds the Hardiness Zones index with options for both Australian and USDA classifications
  • Adds separate indicators for each classification scheme.

Does this PR introduce a breaking change?

No.

Other information:

@Zeitsperre Zeitsperre added the enhancement New feature or request label Jun 16, 2023
@Zeitsperre Zeitsperre self-assigned this Jun 16, 2023
@github-actions github-actions bot added the indicators Climate indices and indicators label Jun 16, 2023
tests/test_indices.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the docs Improvements to documenation label Jun 16, 2023
@Zeitsperre Zeitsperre added this to the v0.44 milestone Jun 16, 2023
@Zeitsperre Zeitsperre mentioned this pull request Jun 19, 2023
5 tasks
@Zeitsperre Zeitsperre requested a review from huard June 19, 2023 17:48
xclim/indices/_agro.py Outdated Show resolved Hide resolved
xclim/data/fr.json Outdated Show resolved Hide resolved
xclim/indices/_agro.py Outdated Show resolved Hide resolved
tests/test_indices.py Outdated Show resolved Hide resolved
docs/references.bib Outdated Show resolved Hide resolved
xclim/indicators/atmos/_temperature.py Outdated Show resolved Hide resolved
xclim/indices/_agro.py Outdated Show resolved Hide resolved
xclim/indices/generic.py Outdated Show resolved Hide resolved
@coxipi
Copy link
Contributor

coxipi commented Jun 21, 2023

In get_zones, zone_min, zone_max and step_size could have conflicting values if (zone_max - zone_min)/step_size is not a integer. Should I raise an error, or give a warning and extend zone_max?

@coxipi
Copy link
Contributor

coxipi commented Jun 21, 2023

Also, I'm thinking we could maybe allow input bins instead of creating them in the function. It might cover some cases where users don't want linearly spaced zones.

@Zeitsperre
Copy link
Collaborator Author

Also, I'm thinking we could maybe allow input bins instead of creating them in the function. It might cover some cases where users don't want linearly spaced zones.

This would make the "Heat Zones" Index (uses non-linearly-spaced bins) very simple to support. Why not? I vote to go for it.

@@ -945,7 +945,7 @@ def get_zones(
Size of zones
zone_1 : Quantified | None
Input value whose zone should be labelled as "zone 1". It should be in the range [`zone_min`, `zone_max` [.
By default this is set to `None`, with "zone 1" starting `zone_min`
By default this is set to `None`, with "zone 1" aligned with `zone_min`
bins : Quantified | None
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this accept Quantified or xr.DataArray | list[Quantity]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the same?

Copy link
Contributor

@coxipi coxipi Jun 22, 2023

Choose a reason for hiding this comment

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

It should be the same as a threshold in other indices e.g. "20 degC" and Quantified seems to be used in this context

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh geez, maybe. I keep getting confused by that variable type. Hold on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh sorry! I'm speaking specifically to bins!

I can see that Quantified is xr.DataArray | str | Quantity, but if we want to provide the option for users setting their own bins, this would be provided as list[str | Quantitiy], no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we're using Quantity and Quantified interchangeably here. I think having a list of bins with different units should be acceptable (or, at the very least, supported).

My understanding here is that we can potentially accept the following:

  1. xr.DataArray
  2. list[str]["5 degC", "10 degC", "30 degF", "20 K"]
  3. list[pint.Quantity][5 * pint.ureg.kelvin, 5 * pint.ureg.celsius]

I hate the 3rd option, but my understanding is that it is technically supported by convert_units_to(). Is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping zone_min, zone_max makes sense, invoke them as we do with thresholds, allow different units.

For bins, xr.DataArray makes more sense. List would be a nice shorthand, but not essential. list[Quantity] seems like a generalization of zone_min , zone_max, zone_step to [zone_a, zone_b, ..., zone_z], but at this point, I'm not sure if it could benefit anybody. I think most people would use: xr.DataArray([value_a,value_b, ...,], attrs={"units":units})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. I think it's probably enough to support zone_min and zone_max with different units, but we should ask that bins consistently use the same unit. It's very difficult to imagine cases where a user would want something more than that.

We can let them open up an issue if ever that happens, which will likely be "never" haha

Copy link
Contributor

@coxipi coxipi Jun 22, 2023

Choose a reason for hiding this comment

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

Ok, let me check what is done elsewhere in the code. I just have the feeling this would be much more flexible than what is otherwise allowed elsewhere, but maybe it's not comparable

EDIT: I don't think there is something comparable really?

Copy link
Contributor

@coxipi coxipi Jun 22, 2023

Choose a reason for hiding this comment

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

Ok, implemented bins : xr.DataArray | list[Quantity] | None.

I agree now that a simple list without units was not a good idea.

As for xr.DataArray vs. xr.DataArray | list[Quantity], I think there is merit to both. We can build indicators with Optional[xr.DataArray] and use bins in this way later. Do we generally aim to limit the generic functions only to inputkinds accepted in indicators? If so, then maybe yes, it's a violation of this rule that does not bring much.

Anyways, once the list of Quantities is converted, I sort it, assuming a user could input a list of bins not necessarily ordered.

I can revert changes if we don't agree on this.

@Zeitsperre Zeitsperre requested a review from huard June 22, 2023 18:27
xclim/indices/generic.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Jun 22, 2023
xclim/indices/_agro.py Outdated Show resolved Hide resolved
@coxipi coxipi merged commit b58b659 into master Jun 22, 2023
14 checks passed
@coxipi coxipi deleted the hardiness-zones branch June 22, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved for additional tests docs Improvements to documenation enhancement New feature or request indicators Climate indices and indicators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants