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

Replace DataAxis and BinnedDataAxisclasses by MapAxis #2133

Merged
merged 23 commits into from May 9, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented May 7, 2019

This PR removes the DataAxis and BinnedDataAxis objects and replaces it with MapAxis. The transition worked without any larger issues. Conceptionally it is a bit weird that the NDDataArray now has a MapAxis, but the functionality is exactly equivalent and the MapAxis also has the better and more mature API. I think it's a huge advantage, that users as well developers have to learn only one single API.

@adonath adonath added the cleanup label May 7, 2019
@adonath adonath added this to the 0.12 milestone May 7, 2019
@adonath adonath self-assigned this May 7, 2019
@adonath adonath added this to To do in gammapy.irf via automation May 7, 2019
@adonath adonath requested review from AtreyeeS and registerrier May 7, 2019
Copy link
Member

@AtreyeeS AtreyeeS left a comment

I agree that this is a nice change. Only 1 API for users to take care of. No comments from my side.

@adonath adonath force-pushed the refactor_nddata_axis branch from 8ed881b to d2da406 May 8, 2019
Copy link
Contributor

@registerrier registerrier left a comment

There is a small typo where offset and energy are mixed.

To limit code, it might be of use to add .lo and .hi properties on the MapAxis. Also an helper function to create an edge array from the lo and hi arrays in input can be useful.

@@ -203,11 +208,14 @@ def to_2d(self):
idx_lat = self.data.axis("fov_lat").find_node("0 deg")[0]
data = self.data.data[:, idx_lon:, idx_lat].copy()

energy = self.data.axis("energy").edges
offset = self.data.axis("energy").edges
Copy link
Contributor

@registerrier registerrier May 8, 2019

Choose a reason for hiding this comment

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

This must be incorrect here. No?
This does not seem to be tested.

Copy link
Member Author

@adonath adonath May 8, 2019

Choose a reason for hiding this comment

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

Good catch, I'll add a regression test as well.

]
self.data = NDDataArray(axes=axes, data=data, interp_kwargs=interp_kwargs)

e_edges = np.append(energy_lo, energy_hi[-1]).value * energy_lo.unit
Copy link
Contributor

@registerrier registerrier May 8, 2019

Choose a reason for hiding this comment

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

Does this work if energy_lo and energy_hi are not given in the same unit?

Copy link
Member Author

@adonath adonath May 9, 2019

Choose a reason for hiding this comment

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

Indeed it doesn't, I have to fix this.

For me the general question was whether we want to support the case with separated edges_mins and edges_maxs on the MapAxis object. I think it mainly exists for "historical" reasons and is basically only used for IRFs, but I'd be OK to add it. So we could include the following:

  • Add MapAxis.edges_mins and MapAxis.edges_maxs (I prefer this names slightly over .lo and .hi, because we have the .edges attribute and the _min, _max convention for maps FITS I/O as well as flux points.
  • Either add a MapAxis.from_edges(edges, edges_maxs=None) option or add a new MapAxis.from_edges_mins_maxs() (which is a bit ugly name...)

Do you have any preference here @AtreyeeS @registerrier?

Copy link
Member Author

@adonath adonath May 9, 2019

Choose a reason for hiding this comment

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

For now I've decided to implement a helper function edges_from_lo_hi. Maybe we can re-discuss the question whether to add this functionality to MapAxis in a different PR. I'll make a reminder issue.

Copy link
Member Author

@adonath adonath May 9, 2019

Choose a reason for hiding this comment

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

See #2134.

energy_axis = MapAxis.from_edges(e_edges, interp="log", name="energy")

# TODO: for some reason the H.E.S.S. DL3 files contain the same values for offset_hi and offset_lo
if np.allclose(offset_lo.to_value("deg"), offset_hi.to_value("deg")):
Copy link
Contributor

@registerrier registerrier May 8, 2019

Choose a reason for hiding this comment

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

Why is the if else statement required now?

Copy link
Member Author

@adonath adonath May 9, 2019

Choose a reason for hiding this comment

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

As far as I understood the DL3 files contain the center values for the offset bins. As this is not supported by the current DL3 data format, the same (center) values are written in offset_hi and offset_lo columns of the FITS file. That's why I added this check...or did you mean something else?

self.data = NDDataArray(axes=axes, data=data, interp_kwargs=interp_kwargs)

e_true_edges = np.append(e_true_lo, e_true_hi[-1]).value * e_true_lo.unit
e_true_axis = MapAxis.from_edges(e_true_edges, interp="log", name="e_true")
Copy link
Contributor

@registerrier registerrier May 8, 2019

Choose a reason for hiding this comment

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

Isn't it worth having a MapAxis.lo and a MapAxis.hi property?

Copy link
Member Author

@adonath adonath May 9, 2019

Choose a reason for hiding this comment

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

See comment above.

]


e_true_edges = np.append(e_true_lo, e_true_hi[-1]).value * e_true_lo.unit
Copy link
Contributor

@registerrier registerrier May 8, 2019

Choose a reason for hiding this comment

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

Maybe all these lines could be replaced by a simple helper function. This could allow to add a check to ensure that lo and hi values are contiguous.

Copy link
Member Author

@adonath adonath May 9, 2019

Choose a reason for hiding this comment

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

See my comment above.

"""Copy `MapAxis` object"""
return copy.deepcopy(self)
def _init_copy(self, **kwargs):
"""Init map instance by copying missing init arguments from self.
Copy link
Contributor

@registerrier registerrier May 8, 2019

Choose a reason for hiding this comment

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

It is MapAxisrather than mapno?

Copy link
Member Author

@adonath adonath May 8, 2019

Choose a reason for hiding this comment

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

Done

@adonath
Copy link
Member Author

@adonath adonath commented May 9, 2019

@registerrier I'll go ahead and merge this PR now. For your remaining comment I opened a separate issue #2134.

@adonath adonath merged commit e4a24d0 into gammapy:master May 9, 2019
9 checks passed
gammapy.irf automation moved this from To do to Done May 9, 2019
@adonath adonath changed the title Replace DataAxis and BinnedDataAxis by MapAxis Replace DataAxis and BinnedDataAxisclasses by MapAxis May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.irf
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants