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

RFC: simplify base CoordinateHandler implementation (cached properties) #4435

Merged

Conversation

neutrinoceros
Copy link
Member

PR Summary

Some low-hanging simplifications I found while working on #4179
functools.cached_property wasn't available before Python 3.8 which explains why these properties didn't use it already.

As a side bonus, I also discovered and replaced two occurrences of abc.abstractproperty, which is deprecated since Python 3.3

@neutrinoceros neutrinoceros added the refactor improve readability, maintainability, modularity label May 3, 2023
@neutrinoceros neutrinoceros marked this pull request as draft May 3, 2023 21:00
@neutrinoceros neutrinoceros force-pushed the rfc_simplify_CoordinateHandler branch from e65db54 to 4c5c6ce Compare May 3, 2023 21:13
@neutrinoceros neutrinoceros marked this pull request as ready for review May 3, 2023 21:32
@matthewturk
Copy link
Member

functools.cached_property wasn't available before Python 3.8 which explains why these properties didn't use it already.

well also even if I wrote them today I didn't know it was in functools so ...

Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

lgtm

dpj[ax] = None
self._data_projection = dpj
return dpj
return {ax: None for ax in self.axis_order}
Copy link
Member

Choose a reason for hiding this comment

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

So this is definitely not a user-facing issue, but from my perspective, I am wondering -- is it possible to invalidate the cached property at runtime?

Copy link
Member Author

Choose a reason for hiding this comment

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

The result of a cached_property can be mutated transparently. Does this answer your question ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I guess you were wondering if it was possible to reset the property to its initial state.
In which case the answer is: not to my knowledge

Copy link
Member Author

Choose a reason for hiding this comment

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

... and a better answer yet, from the docs:

The cached value can be cleared by deleting the attribute. This allows the cached_property method to run again.

@neutrinoceros neutrinoceros merged commit 597b509 into yt-project:main May 4, 2023
12 checks passed
@neutrinoceros neutrinoceros deleted the rfc_simplify_CoordinateHandler branch May 4, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor improve readability, maintainability, modularity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants