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

Implement WcsGeom coord caching #2377

Merged
merged 8 commits into from Sep 20, 2019
Merged

Implement WcsGeom coord caching #2377

merged 8 commits into from Sep 20, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented Sep 19, 2019

This PR implements caching of coordinates / derived quantities on the WCSGeom object, by using the lru_cache decorator form the functools standard library. It implements the following changes:

  • Make the WcsGeomhashable
  • Make the MapAxis hashable
  • Make MapAxis.edges and MapAxis.center a lazy property
  • Cache WcsGeom.get_coord(), WcsGeom.get_pix() WcsGeom.get_idx() and WcsGeom.solid_angle()

This PR addresses #1848.

@adonath adonath self-assigned this Sep 19, 2019
@adonath adonath added this to the 0.14 milestone Sep 19, 2019
@adonath adonath force-pushed the wcs_geom_caching branch from 27ec1cd to 5347e83 Sep 19, 2019
@adonath adonath force-pushed the wcs_geom_caching branch from 5347e83 to ec365a7 Sep 19, 2019
Copy link
Member

@cdeil cdeil left a comment

@adonath - Thanks!

One question inline. Custom __hash__ is new and advanced in Gammapy, so we should have a good understanding of it and find a good pattern to write those. (I'm assuming we will add more, as we add more caching within Gammapy to improve performance in the coming months)

@@ -400,6 +389,9 @@ def __eq__(self, other):
def __ne__(self, other):
return not self.__eq__(other)

def __hash__(self):
return hash((self._nodes.tostring(), self._node_type, self._name))
Copy link
Member

@cdeil cdeil Sep 20, 2019

Choose a reason for hiding this comment

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

What is the logic of which properties are hashed here? Why isn't e.g. unit included?
For the nodes array: can't we use the Python object ID, avoiding the tostring computation?
I think the assumption is anyways that no-one messes with the nodes array values after init, no?

Copy link
Member Author

@adonath adonath Sep 20, 2019

Choose a reason for hiding this comment

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

Adressed. As discussed in the dev meeting I changed to using id() as a hash.

cdeil
cdeil approved these changes Sep 20, 2019
Copy link
Member

@cdeil cdeil left a comment

LGTM

@cdeil cdeil merged commit 21afa02 into gammapy:master Sep 20, 2019
9 checks passed
@adonath adonath mentioned this pull request Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants