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

heat_index with list comprehension to address #596 #597

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

anissa111
Copy link
Member

@anissa111 anissa111 commented Apr 16, 2024

PR Summary

Closes #596

Right, so what I've done here is switched from using xr.where to handle the adjustments to a (admittedly weighty) list comprehension.

The list comprehension makes it so that the adjustment won't be calculated where the value doesn't meet the adjustment criteria. The adjustment calculation only tries to square root a negative outside of the adjustment criteria, so this eliminates that possibility. I've also added an extra test that makes sure dask works with multi-dimensional inputs.

I am not at all sure that my usage of dask here is ideal, but it does work? If anybody has any suggestions, I'd be happy to hear them.


PR Checklist

General

  • Make an issue if one doesn't already exist
  • Link the issue this PR resolves by adding closes #XXX to the PR description where XXX is the number of the issue.
  • Add a brief summary of changes to docs/release-notes.rst in a relevant section for the next unreleased release. Possible sections include: Documentation, New Features, Bug Fixes, Internal Changes, Breaking Changes/Deprecated
  • Add appropriate labels to this PR
  • Make your changes in a forked repository rather than directly in this repo
  • Open this PR as a draft if it is not ready for review
  • Convert this PR from a draft to a full PR before requesting reviewers
  • Passes precommit. To set up on your local, run pre-commit install from the top level of the repository. To manually run pre-commits, use pre-commit run --all-files and re-add any changed files before committing again and pushing.
  • If needed, squash and merge PR commits into a single commit to clean up commit history

@anissa111
Copy link
Member Author

Well dang it, I broke dask.

@anissa111 anissa111 added bug Something isn't working refactor Internal code refactoring testing Issue related to testing labels Apr 16, 2024
@anissa111 anissa111 marked this pull request as ready for review April 17, 2024 17:29
@anissa111 anissa111 requested review from kafitzgerald, erogluorhan and cyschneck and removed request for kafitzgerald April 17, 2024 17:29
Copy link
Contributor

@kafitzgerald kafitzgerald left a comment

Choose a reason for hiding this comment

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

I'm a little worried about performance (especially with Xarray/Dask) and readability here.

I did some quick testing of this proposed change with larger arrays and saw some pretty major impacts to performance when using Xarray objects. I've not written a benchmark (yet), but I can work on that. The point being though that I think we should look into other options.

Unfortunately, I don't have a nice clean solution that solves for everything.

I did look into rewriting this as a ufunc last week, but wasn't having much luck avoiding the original issue.

Rewriting the np.sqrt as **0.5 does avoid the warning for xarray object, but not for numpy. This is how MetPy does it and we could potentially filter the remaining warning if that's bothersome. That said, I realize this doesn't address the additional calculations issue, but I'm struggling to come up with something that does and plays well with Xarray and Dask.

@anissa111
Copy link
Member Author

Sigh, yeah I unfortunately agree. I'm going to draft-mode this and go back to the drawing board.

I'm a little worried about performance (especially with Xarray/Dask) and readability here.

I did some quick testing of this proposed change with larger arrays and saw some pretty major impacts to performance when using Xarray objects. I've not written a benchmark (yet), but I can work on that. The point being though that I think we should look into other options.

Unfortunately, I don't have a nice clean solution that solves for everything.

I did look into rewriting this as a ufunc last week, but wasn't having much luck avoiding the original issue.

Rewriting the np.sqrt as **0.5 does avoid the warning for xarray object, but not for numpy. This is how MetPy does it and we could potentially filter the remaining warning if that's bothersome. That said, I realize this doesn't address the additional calculations issue, but I'm struggling to come up with something that does and plays well with Xarray and Dask.

@anissa111 anissa111 marked this pull request as draft April 22, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactor Internal code refactoring testing Issue related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

heat_index encounters sqrt of negatives
3 participants