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

Refactor energy grouping #2114

Merged
merged 8 commits into from Apr 16, 2019
Merged

Refactor energy grouping #2114

merged 8 commits into from Apr 16, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented Apr 15, 2019

This PR refactors the energy grouping for the flux points computation. It includes the following changes:

  • Add a MapAxis.group_table method. This replaces the SpectrumEnergyGroupMAker.compute_fixed method. It just returns a Table object that defines the energy groups
  • Add Quantity support to MapAxis.from_edges() and MapAxis.from_nodes()
  • Change the FluxPointEstimator to directly take e_edges on init.
  • Remove the SpectrumEnergyGroup, SpectrumEnergyGroups and the SpectrumEnergyGroupMaker classes.
  • Adapt test and tutorials
@adonath adonath self-assigned this Apr 15, 2019
@adonath adonath added this to the 0.12 milestone Apr 15, 2019
@adonath adonath added this to To do in gammapy.makers via automation Apr 15, 2019
@adonath adonath requested review from kbruegge and registerrier Apr 15, 2019
Copy link
Contributor

@registerrier registerrier left a comment

Thanks @adonath. Looks good!

It is fine to remove EnergyGroupMaker since it was not implementing sophisticated determination of the bounds, e.g. minimal SNR or minimal counts.
In a sense, this can be performed only by either the CountsSpectrum or the SpectrumDatasetOnOff itself.

@@ -483,7 +484,12 @@ def from_nodes(cls, nodes, **kwargs):
Interpolation method used to transform between axis and pixel
coordinates. Default: 'lin'.
"""
nodes = np.array(nodes, ndmin=1)
if isinstance(nodes, u.Quantity):
Copy link
Contributor

@registerrier registerrier Apr 15, 2019

OK. This is a nice addition, but one might wonder if this shouldn't be on the constructor itself?

Copy link
Member Author

@adonath adonath Apr 15, 2019

Good point, I'll move it to __init__.




class TestMapAxisGropuping:
Copy link
Contributor

@registerrier registerrier Apr 15, 2019

TestMapAxisGrouping:

Copy link
Contributor

@registerrier registerrier Apr 15, 2019

If you have only static methods, why do you need to make this a class?

Copy link
Member Author

@adonath adonath Apr 15, 2019

Good question! I think I just prefer to group related tests, but as there is no MapAxisGrouping class, there is no real point. I will change it...

def e_groups(self):
""""""
from ..maps import MapAxis
e_edges = self.datasets.datasets[0].counts_on.energy.bins
Copy link
Contributor

@registerrier registerrier Apr 15, 2019

This really calls for CountsSpectrum to use a MapAxis rather than a BinnedNDAxis. But this will be for a later PR I can imagine.

Copy link
Member Author

@adonath adonath Apr 15, 2019

Yes, ideally we can reuse the MapAxis here in future.

@@ -640,6 +651,69 @@ def copy(self):
return copy.deepcopy(self)


def group_table(self, edges):
Copy link
Contributor

@registerrier registerrier Apr 15, 2019

Is there a use case for other MapAxis than energy axis to perform this type of grouping? Should there be a specialized EnergyAxis for this?

Copy link
Member Author

@adonath adonath Apr 15, 2019

I was thinking maybe for the time binning, but there the logic will probably be different, because time bins are probably represent by a single dataset each. I agree, if we introduce a specialised EnergyAxis class, this method should go there.

@adonath
Copy link
Member Author

@adonath adonath commented Apr 15, 2019

Thanks @registerrier for your comments. Yes, I decided to remove the SpectrumEnergyGroupMaker, for now, because it didn't offer anything useful, but just "cluttered" the API of the FluxPointEstimator.
I also thought it was overcomplicated to have two data classes SpectrumEnergyGroup and SpectrumEnergyGroups, if the same information can be represented as a table.

For the adaptive binning methods, we'll see what to do in future. Either we'll directly implement them on the FluxPointEstimator or we re-introduce an EnergyGroupEstimator, which will directly work on the SpectrumDataset or MapDataset then.

@adonath adonath merged commit af8130e into gammapy:master Apr 16, 2019
9 checks passed
gammapy.makers automation moved this from To do to Done Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants