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

Avoid extra read from disk when creating Pandas Index. #8893

Merged
merged 3 commits into from
May 7, 2024
Merged

Avoid extra read from disk when creating Pandas Index. #8893

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a40701
Avoid extra read from disk when creating Pandas Index.
dcherian Mar 29, 2024
80b04fe
Update xarray/core/indexes.py
dcherian Apr 5, 2024
38c291c
Merge branch 'main' into optimize-pd-indx
dcherian Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion xarray/core/indexes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ def from_variables(
# the checks below.

# preserve wrapped pd.Index (if any)
data = getattr(var._data, "array", var.data)
data = getattr(var._data, "array") if hasattr(var._data, "array") else var.data
Copy link
Member

Choose a reason for hiding this comment

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

can you explain what is going on here? Why does var.data trigger an additional load? Where does our cache mechanism come in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var._data contains array(array(array(...))) . We load these on the first access to .data and .values

This is a regression from Anderson's work on indexing I think. Something in the pipeline was reading this earlier and so it was fine at this point. Now we must be more careful about loading and this line was loading it again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I being slow or do these two lines have exactly the same semantics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but else conditions don't get evaluated unless necessary

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry, I was being slow

Copy link
Collaborator

Choose a reason for hiding this comment

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

In penance I add a comment

Suggested change
data = getattr(var._data, "array") if hasattr(var._data, "array") else var.data
# accessing `.data` can load data from disk, so we only access if needed
data = getattr(var._data, "array") if hasattr(var._data, "array") else var.data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry the ternary thing is a bit cute :) The comment makes it better.

Copy link
Collaborator

@keewis keewis Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hasattr(obj, name) is currently implemented as something similar to:

try:
    getattr(obj, name)
    return True
except AttributeError:
    return False

so properties will still be evaluated. If you don't want that, use inspect.getattr_static.

Edit: probably not relevant since _data is a simple attribute. But still good to know!

# multi-index level variable: get level index
if isinstance(var._data, PandasMultiIndexingAdapter):
level = var._data.level
Expand Down
Loading