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

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

Merged
merged 3 commits into from
May 7, 2024

Conversation

dcherian
Copy link
Contributor

No description provided.

@dcherian dcherian requested a review from benbovy March 29, 2024 17:44
@@ -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

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!

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
@dcherian dcherian added the run-slow-hypothesis Run slow hypothesis tests label Apr 5, 2024
@dcherian
Copy link
Contributor Author

dcherian commented Apr 5, 2024

I'd like to make sure we only read a variable at most once in our backend test, potentially with some mocking but I won't have time to get to it soon.

Thoughts on just merging as-is?

@dcherian
Copy link
Contributor Author

dcherian commented May 7, 2024

My attempts to add some kind of generic mocking did not succeed. I'll punt that to a future date.

@dcherian dcherian merged commit 322e670 into pydata:main May 7, 2024
29 of 31 checks passed
@dcherian dcherian deleted the optimize-pd-indx branch May 7, 2024 19:28
andersy005 pushed a commit that referenced this pull request May 10, 2024
* Avoid extra read from disk when creating Pandas Index.

* Update xarray/core/indexes.py

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

---------

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
andersy005 added a commit that referenced this pull request May 10, 2024
* main:
  Avoid auto creation of indexes in concat (#8872)
  Fix benchmark CI (#9013)
  Avoid extra read from disk when creating Pandas Index. (#8893)
  Add a benchmark to monitor performance for large dataset indexing (#9012)
  Zarr: Optimize `region="auto"` detection (#8997)
  Trigger CI only if code files are modified. (#9006)
  Fix for ruff 0.4.3 (#9007)
  Port negative frequency fix for `pandas.date_range` to `cftime_range` (#8999)
  Bump codecov/codecov-action from 4.3.0 to 4.3.1 in the actions group (#9004)
  Speed up localize (#8536)
  Simplify fast path (#9001)
  Add argument check_dims to assert_allclose to allow transposed inputs (#5733) (#8991)
  Fix syntax error in test related to cupy (#9000)
andersy005 added a commit to hmaarrfk/xarray that referenced this pull request May 10, 2024
* backend-indexing:
  Trigger CI only if code files are modified. (pydata#9006)
  Enable explicit use of key tuples (instead of *Indexer objects) in indexing adapters and explicitly indexed arrays (pydata#8870)
  add `.oindex` and `.vindex` to `BackendArray` (pydata#8885)
  temporary enable CI triggers on feature branch
  Avoid auto creation of indexes in concat (pydata#8872)
  Fix benchmark CI (pydata#9013)
  Avoid extra read from disk when creating Pandas Index. (pydata#8893)
  Add a benchmark to monitor performance for large dataset indexing (pydata#9012)
  Zarr: Optimize `region="auto"` detection (pydata#8997)
  Trigger CI only if code files are modified. (pydata#9006)
  Fix for ruff 0.4.3 (pydata#9007)
  Port negative frequency fix for `pandas.date_range` to `cftime_range` (pydata#8999)
  Bump codecov/codecov-action from 4.3.0 to 4.3.1 in the actions group (pydata#9004)
  Speed up localize (pydata#8536)
  Simplify fast path (pydata#9001)
  Add argument check_dims to assert_allclose to allow transposed inputs (pydata#5733) (pydata#8991)
  Fix syntax error in test related to cupy (pydata#9000)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-slow-hypothesis Run slow hypothesis tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants