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 Energy and EnergyBounds classes #2237

Merged
merged 5 commits into from Jun 15, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented Jun 14, 2019

This PR removes the Energy and EnergyBounds classes from gammapy.utils.energy and all of its use in Gammapy. In 90% of the cases the classes where used to create a log-spaced array of energies. This functionality I still considered useful and moved to a helper function energy_logspace. The name and place for this helper function is open for discussion.

In some places this change took away a little bit of convenience (e.g. [1:] instead of .upper_bounds) and np.sqrt(energy[1:] * energy[:-1]) instead of .log_center. But this convenience does not justify maintaining the ~500 lines of code we had for the classes internally.

@adonath adonath self-assigned this Jun 14, 2019
@adonath adonath added this to the 0.13 milestone Jun 14, 2019
Copy link
Contributor

@registerrier registerrier left a comment

Thanks @adonath .
I would simply add a test for energy_logspace.

@cdeil
Copy link
Member

@cdeil cdeil commented Jun 14, 2019

@adonath - Thank you for doing clean-up on this!

There's a few things I'm not sure about. Proceed as you (and @registerrier) think best.

  • Is energy_logspace(...) useful enough? Or just use Axis(...).quantity?
  • The np.sqrt(energy[1:] * energy[:-1]) in ~ 10 places in the code is not obvious to everyone. Use a 1-line helper function for this? Or again, co-opt Axis somehow for this, i.e. something like Axis(ebounds).log_center?

If we don't make Axis the class to work with logspace and log_center, then maybe it would be OK to keep these two functions in a class?

class Energy(Quantity):
    def from_logspace(...):
        ...

    def log_center(...):
        ...

It wouldn't be part of the interfaces in Gammapy, it would be a little helper to do these computations within Gammapy.

The difference between exposing two functions in gammapy.utils.energy or on the class gammapy.utils.energy.Energy is small, I'm not sure if the introduction of Energy was a mistake, maybe it was just written in a much too complex way?

For EnergyBounds I feel like you - it's best to remove it, and just make it clear when one is dealing with energy bounds via variable names.

To summarise: suggest to either use Axis more (currently called MapAxis, would then have to be moved to utils and renamed if used everywhere in Gammapy), or to add Energy.log_center or energy_log_center helper instead of the raw np.sqrt(energy[1:] * energy[:-1]) in many places.

@adonath
Copy link
Member Author

@adonath adonath commented Jun 15, 2019

@cdeil I agree we should try to go "all-in" with the MapAxis. All use cases we have for energy_logspace and .log_center can be solved with the MapAxis object. The cases where we now have the np.sqrt(energy[:-1] * energy[1:]) are to a large part in gammapy.irf, where we should indeed try to also use the MapAxis more consistently. I'll try to improve the PR in this regard.

I'm not sure either whether introducing the Energy class was a mistake, but my impression was that the motivation was weak and mostly "over-convenience" for users. I think in one phase of developing Gammapy we had the tendency to introduce new "convenience" classes for everything, instead of relying on good existing solutions (quantities, dicts, tables, etc.) and teach users how to work with those instead. I feel e.g. similar about those ObservationStats or SpectrumStats or the SpectrumEnergyGroup and SpectrumEnergyGroups classes we had. Now I have a clear tendency to rather focus on O(10) central classes and try to avoid a proliferation of helper classes.

@adonath
Copy link
Member Author

@adonath adonath commented Jun 15, 2019

I added a helper method energy_logcenter() and test as well as corresponding tests for energy_logspace(). Will go ahead and merge now.

@adonath adonath merged commit fac710c into gammapy:master Jun 15, 2019
9 checks passed
@cdeil
Copy link
Member

@cdeil cdeil commented Jun 16, 2019

@adonath - If you look at the diff at https://github.com/gammapy/gammapy/pull/2237/files you'll see that there's still 5 cases where np.sqrt is added to compute an energy logcenter. Could you please change those to use your energy_logcenter, to have clear and uniform code (it's not obvious to think "log center" when one sees "sqrt")?

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