Skip to content

New energy axis computation#93

Open
gabemery wants to merge 2 commits into
mainfrom
new_energy_axis_computation
Open

New energy axis computation#93
gabemery wants to merge 2 commits into
mainfrom
new_energy_axis_computation

Conversation

@gabemery

Copy link
Copy Markdown
Collaborator

Allows for a different computation and base axis, even with the dynamic energy binning.
Stop overwriting the computation axis with the dynamic one to allow better reuse of the class objects and a posteriori extraction of starting informations.

close #86

@gabemery gabemery added this to the v0.5.0 milestone Jun 25, 2026
@gabemery
gabemery requested review from MSCarrasco and mdebony June 25, 2026 15:08
@gabemery gabemery self-assigned this Jun 25, 2026

@mdebony mdebony left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

My main comment is on naming. When reviewing the PR it was a bit confusing sometime. The best idea I have right now is to be explicit everywhere, so to no have energy_axis_computation anymore, but a base and an effective. However I agree it's not the best, so if you have better idea, feel free to do it.

Comment on lines +187 to +192
@property
def energy_axis_computation(self):
if self.dynamic_energy_axis:
return self._energy_axis_computation
else:
return self.base_energy_axis_computation

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Would not be simpler to simply initialise self._energy_axis_computation at the same time we initialise self.base_energy_axis_computation

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My idea here is that someone could decide to deactivate the dynamic energy binning after the class init. And my logic for not initialising it at least at the start is that I want it to be easy to know if the dynamic binning was run already.

@gabemery

Copy link
Copy Markdown
Collaborator Author

@mdebony what about, for the naming:

  • energy_axis -> target_energy_axis / out(put)_energy_axis / irf_energy_axis ?
  • energy_axis_computation -> keep it as the access name when used
  • _energy_axis_computation -> _dynamic_energy_axis
  • base_energy_axis_computation -> base_energy_axis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Not intuitive use of the energy_axis_computation with dynamic energy binning

2 participants