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

Human readable energy units string formatting for plot_interactive & plot_grid #3752

Merged
merged 23 commits into from Apr 15, 2022

Conversation

facero
Copy link
Contributor

@facero facero commented Jan 27, 2022

Description
When using data on a large energy range or for the example the Crab dataset, the energy label are not the most readable (e.g. 2.56e8 keV instead of 256 GeV).
This PR adds a helper function in utils to transform a string, list/np array of energy Quantities to a string or list of strings of well formatted energies.
Example:
E = [1.54e2 *u.GeV, 4300 u.keV, 300.6e12u.eV]
=> ['154 GeV', '4.30 MeV', '301 TeV']

Example of before/After in plot_interactive:
image

Dear reviewer
This helper function has been applied to the plot_interactive() and plot_grid() functions to show more readable energies.
The function could also be useful in other contexts but I started with these only 2 cases for now.

Tests were added as well.

One point I'm not sure is how to best check whether the input is iterable or not in energy_unit_format().
Possible inputs are list of quantities or a np.array of quantities.
If I have just E = 1 *u.TeV I don't to return a list.
Any suggestions to improve my first attempt ?

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #3752 (e0e55e9) into master (e4536a9) will increase coverage by 0.01%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master    #3752      +/-   ##
==========================================
+ Coverage   93.77%   93.78%   +0.01%     
==========================================
  Files         162      162              
  Lines       19576    20125     +549     
==========================================
+ Hits        18357    18874     +517     
- Misses       1219     1251      +32     
Impacted Files Coverage Δ
gammapy/maps/core.py 86.70% <70.00%> (-1.56%) ⬇️
gammapy/utils/units.py 100.00% <100.00%> (ø)
gammapy/irf/rad_max.py 94.44% <0.00%> (-5.56%) ⬇️
gammapy/catalog/core.py 94.81% <0.00%> (-3.56%) ⬇️
gammapy/makers/utils.py 96.95% <0.00%> (-1.85%) ⬇️
gammapy/data/gti.py 98.31% <0.00%> (-0.78%) ⬇️
gammapy/datasets/flux_points.py 93.25% <0.00%> (-0.72%) ⬇️
gammapy/makers/safe.py 87.15% <0.00%> (-0.70%) ⬇️
gammapy/catalog/fermi.py 94.72% <0.00%> (-0.57%) ⬇️
... and 77 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

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

Thanks @facero!

While I think it is clearly an improvement of readability in the plots, I must admit I'm not yet happy with the implementation yet. This PR adds ~100 lines of code to achieve this small improvement, which seems a bit "overkill" and it is hard-coded to energy units only. I did a bit of research to see whether there are already similar formatting options available in numpy or astropy, but it doesn't seem to be the case...

Maybe we should open an issue in astropy first and ask the astropy.units experts how to best achieve something like this? I noticed e.g. that astropy handles this as a special PrefixUnit...

gammapy/utils/units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved
facero and others added 6 commits March 16, 2022 10:08
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
handle precision + nearest unit

return f"{v:0.{prec}f} {unit}"

def energy_unit_format(E):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented the very good suggestions from Nathaniel. Now this function energy_unit_format and in maps/core.py could also be optimized a bit. Any suggestions ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative to the suggestion below is to just use a little bit of recursion so this function will work on 1-D arrays (e.g. a list / tuple).

def energy_str_formatting(E):
    
     try:
         iter(E)
     except TypeError:
         pass
     else:
         return tuple(map(energy_str_formatting, E))

     i = floor(np.log10(E.to_value(u.eV)) / 3) # a new unit every 3 decades
     unit = (u.eV, u.keV, u.MeV, u.GeV, u.TeV, u.PeV)[i] if i < 5 else u.PeV        

     v = E.to_value(unit)
     i=floor(np.log10(v))
     prec = (2,1,0)[i] if i < 3 else 0

     return f"{v:0.{prec}f} {unit}"

Copy link
Contributor

Choose a reason for hiding this comment

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

Then energy_unit_format is extraneous.

gammapy/maps/core.py Outdated Show resolved Hide resolved
gammapy/maps/core.py Outdated Show resolved Hide resolved
gammapy/utils/tests/test_units.py Outdated Show resolved Hide resolved
gammapy/utils/tests/test_units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved
facero and others added 10 commits March 25, 2022 16:23
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@facero
Copy link
Contributor Author

facero commented Mar 25, 2022

I've implemented the suggested changes and code is more compact now.
Tests are successful locally (except the known issue related to Regions).
There is still one part that I'm not happy about but don't know if I can do better (see code comment).

gammapy/utils/units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved
gammapy/utils/units.py Outdated Show resolved Hide resolved

return f"{v:0.{prec}f} {unit}"

def energy_unit_format(E):
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative to the suggestion below is to just use a little bit of recursion so this function will work on 1-D arrays (e.g. a list / tuple).

def energy_str_formatting(E):
    
     try:
         iter(E)
     except TypeError:
         pass
     else:
         return tuple(map(energy_str_formatting, E))

     i = floor(np.log10(E.to_value(u.eV)) / 3) # a new unit every 3 decades
     unit = (u.eV, u.keV, u.MeV, u.GeV, u.TeV, u.PeV)[i] if i < 5 else u.PeV        

     v = E.to_value(unit)
     i=floor(np.log10(v))
     prec = (2,1,0)[i] if i < 3 else 0

     return f"{v:0.{prec}f} {unit}"


return f"{v:0.{prec}f} {unit}"

def energy_unit_format(E):
Copy link
Contributor

Choose a reason for hiding this comment

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

Then energy_unit_format is extraneous.

facero and others added 2 commits March 30, 2022 11:15
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@facero
Copy link
Contributor Author

facero commented Mar 30, 2022

Thanks @nstarman for the suggestions.
Especially return tuple(map(energy_str_formatting, E)) is a nice workaround to handle both scalar and iterables.
With this I reduced the code to one function only which also reduced the tests (40 lines now instead of 100 at the start).
@adonath can you have a look at this latest version ?

registerrier
registerrier previously approved these changes Apr 14, 2022
Copy link
Contributor

@registerrier registerrier left a comment

Choose a reason for hiding this comment

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

Thanks @facero . This looks good!

I have left a few minor comments inline.

@@ -1065,10 +1066,15 @@ def plot_grid(self, figsize=None, ncols=3, **kwargs):
image_wcs.plot(ax=ax, **kwargs)

if axis.node_type == "center":
info = f"{axis.center[idx]:.1f}"
if axis.name == "energy":
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also energy_true?

@@ -1129,7 +1135,16 @@ def plot_interactive(self, rc_params=None, **kwargs):
interact_kwargs = {}

for axis in self.geom.axes:
options = axis.as_plot_labels
if axis.node_type == "center":
if axis.name == "energy":
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here check energy_true

"""
Format energy quantities to a string representation that is more comfortable to read
by swithcing to the most relevant unit (keV, MeV, GeV, TeV) and changing the float precision.
Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Add blank line before Parameters

----------
E: `~astropy.units.Quantity`
Quantity or list of quantities
Returns
Copy link
Contributor

Choose a reason for hiding this comment

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

Add blank line

def energy_unit_format(E):
"""
Format energy quantities to a string representation that is more comfortable to read
by swithcing to the most relevant unit (keV, MeV, GeV, TeV) and changing the float precision.
Copy link
Contributor

Choose a reason for hiding this comment

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

switching

@registerrier
Copy link
Contributor

Merging now. Thanks @facero.

@registerrier registerrier merged commit 8e5013c into gammapy:master Apr 15, 2022
@facero facero deleted the plot_interactive branch April 15, 2022 13:00
@adonath adonath modified the milestones: 0.20.1, 0.20 Sep 20, 2022
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.

None yet

4 participants