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

Remove unused map iter_by_pix and iter_by_coord methods #2206

Merged
merged 1 commit into from Jun 5, 2019

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Jun 4, 2019

This PR removes the map iter_by_pix and iter_by_coord methods.

They are unused in Gammapy, no use case has come up for this so far. The use case given in the docs could just as well and much more efficiently be written by using the whole map directly, or by using iter_by_image.

Iterating over pixels in Python will never be efficient, should always be avoided. Almost all computations can be expressed using Numpy, and should that not be possible, one could use Numba or Cython, or if really needed, do this Python pixel loop where it's needed.

IMO we shouldn't leave such code in Gammapy, but start to either remove or improve quality as we work towards Gammapy v1.0. Note that this code is non-trivial and likely not very well written. E.g. the docstring of unpack_seq says that it is probably not needed in Python 3. Also the implementation of the pixel loop contains list(self.geom.get_coord(flat=True)), i.e. a Python list of all pixel coords is made, which takes a lot of memory. If a generator pixel iter were kept, it should not contain such a list inside. So the question is really: OK to remove? Or spend effort to improve before v1.0?

@registerrier @adonath - Thoughts?

@cdeil cdeil added the cleanup label Jun 4, 2019
@cdeil cdeil added this to the 0.13 milestone Jun 4, 2019
@cdeil cdeil self-assigned this Jun 4, 2019
@cdeil cdeil added this to To do in gammapy.maps via automation Jun 4, 2019
@cdeil cdeil requested review from adonath and registerrier Jun 4, 2019
@cdeil
Copy link
Member Author

@cdeil cdeil commented Jun 4, 2019

I forgot to say: these methods were added in #1093 as part of a very large code addition (2500 lines) to Gammapy, and it wasn't discussed at all.

adonath
adonath approved these changes Jun 5, 2019
Copy link
Member

@adonath adonath left a comment

Thanks, @deil. I agree those methods are not very useful. So +1 to remove.

Copy link
Contributor

@registerrier registerrier left a comment

Fine to remove too. It is not used, and I don't see a clear use case for this.

@cdeil cdeil merged commit 70515f9 into gammapy:master Jun 5, 2019
9 checks passed
gammapy.maps automation moved this from To do to Done Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.maps
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants