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 use of NDDataArray from CountsSpectrum #2240

Merged
merged 8 commits into from Jun 17, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented Jun 15, 2019

This PR removes the use of NDDataArray from the CountsSpectrum class and makes CountsSpectrum.data a plain np.ndarray. This simplifies the data access, without loosing any functionality and makes the API more uniform with the Map object.

@adonath adonath self-assigned this Jun 15, 2019
@adonath adonath added this to the 0.13 milestone Jun 15, 2019
@cdeil
Copy link
Member

@cdeil cdeil commented Jun 16, 2019

Docstring says:

    data : `~astropy.units.Quantity`, array-like
        Counts

Actually it was a NDDataArray and now it's a numpy.ndarray and the unit attribute isn't documented.

Shouldn't a spectrum have an energy axis?

OK to go this way if you think it's best, then just fix the class-level docstring.

@adonath
Copy link
Member Author

@adonath adonath commented Jun 17, 2019

Thanks @cdeil! I cleaned-up the docstring and a few more callers to CountSpectrum.data.data. Note the CountsSpectrum has an energy axis (represented by a MapAxis) defined under the CountsSpectrum.energy attribute (that is for historical reasons...).

@cdeil
Copy link
Member

@cdeil cdeil commented Jun 17, 2019

So the energy grid is stored twice, and probably half of the callers use counts.energy and the other counts.energy_lo, counts.energy_hi? Get rid of counts.energy_lo and counts.energy_hi?

@adonath
Copy link
Member Author

@adonath adonath commented Jun 17, 2019

No, CountsSpectrum.energy_hi and CountsSpectrum.energy_lo does not exist, it is just used in __init__, to create a MapAxis object (again for historical reasons...).

@adonath adonath merged commit d024f1d into gammapy:master Jun 17, 2019
4 of 9 checks passed
@registerrier
Copy link
Contributor

@registerrier registerrier commented Jun 17, 2019

Sorry to comment a bit late. Unifying the data access API is indeed a good thing. Another approach, more complex though, could have been to make CountsSpectrum a NDDataArray directly, no?
This is the main difference with our usages of Map, where all the objects are directly Maps. Each time we have a NDDataArray there is another layer of object containing it.

@adonath
Copy link
Member Author

@adonath adonath commented Jun 17, 2019

@registerrier If you take a look at the current state of the code, the NDDataArray does not really offer anything useful. It is used to bundle the data axes and setup the ScaledRegularGridInterpolator. I also think it has the fundamental design mistake of giving up numpy broadcasting in .evaluate(). It's completely clear to me, that the current Map design is much better.

Midterm my much preferred solution would be to get completely rid of NDDataArray and replace it with a Map like data structure, that does not require a spatial axis. This can then be used for spectra and IRFs as well. In that sense the NDDataArray is almost this kind of data structure, maybe it could be refactored as well...

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

3 participants