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.

Sign up for GitHub

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

Jump to bottom

Use compute instead of load in plot #9818

Merged
merged 5 commits into from
Nov 26, 2024
Merged

Use compute instead of load in plot #9818

merged 5 commits into from
Nov 26, 2024

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Nov 24, 2024 •
edited
Loading

As the docstring says, xr.DataArray.load loads the data inplace. So the dataset is affected as well which was not intended.

  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

@mathause
Copy link
Collaborator

Does this need compute on each call of plot? That could make for a bad user experience. (I often have to re-do the same plot and if each iteration needs to load the data from disk this is not pleasant).

@Illviljan
Copy link
Contributor Author

Does this need compute on each call of plot? That could make for a bad user experience. (I often have to re-do the same plot and if each iteration needs to load the data from disk this is not pleasant).

Yes. I see your point but this is how it is normally done inside xarray code and I think inplace loads should be up to the user, not something xarray does.
This .load was very magical to me and I had a difficult time figuring out why my expected dask.array all of a sudden was a numpy array even though I simply did ds[key].data.

There's also a risk of leaking RAM, due to each call loading more and more variables to memory even though it's not needed after the plot has been saved to file.

plot2d also computes early (via .to_numpy) and without the inplace effect:

xarray/xarray/plot/dataarray_plot.py

Lines 1564 to 1566 in a765ae0

# better to pass the ndarrays directly to plotting functions
xvalnp = xval.to_numpy()
yvalnp = yval.to_numpy()

mathause
mathause approved these changes Nov 25, 2024
View reviewed changes
Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

Yes. I see your point but this is how it is normally done inside xarray code and I think inplace loads should be up to the user, not something xarray does.

Fair enough.

@Illviljan Illviljan merged commit dafcde2 into pydata:main Nov 26, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shoyer shoyer shoyer approved these changes

@mathause mathause mathause approved these changes

Assignees
No one assigned
Labels
topic-plotting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Illviljan @mathause @shoyer