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

Review and unify quantity handling #1898

Merged
merged 15 commits into from Nov 6, 2018

Conversation

Projects
None yet
3 participants
@adonath
Member

adonath commented Nov 2, 2018

This PR reviews, unifies and improves the quantity handling in Gammapy. It addresses also #1705. I've decided to also implement the .to_value() pattern suggest by @cdeil.

@adonath adonath added the cleanup label Nov 2, 2018

@adonath adonath added this to the 0.9 milestone Nov 2, 2018

@adonath adonath self-assigned this Nov 2, 2018

@registerrier

This comment has been minimized.

Contributor

registerrier commented Nov 2, 2018

Hi @adonath . Nice, thanks!
I note that there will be a few conflicts with PRs #1892 , #1893. Normally those should be ready to merge. Do you mind if I merge then in first?

@adonath

This comment has been minimized.

Member

adonath commented Nov 2, 2018

@registerrier OK let's merge those first and I'll try to resolve the conflicts in this PR.

val = np.select([sep < r_i, sep < r_o], [val_in, val_out])
# np.where and np.select do not work with quantities, so we use the
# workaround with indexing
value = np.sqrt(radius_out ** 2 - sep ** 2)

This comment has been minimized.

@registerrier

registerrier Nov 3, 2018

Contributor

How much slower is evaluation with quantities compared to regular floats?
Isn't it better to have the .to_value()at the beginning and just multiply with the correct unit at the end?

This comment has been minimized.

@adonath

adonath Nov 5, 2018

Member

See my comment above. I've decided to rewrite the .evaluate() methods in a way, that it works for np.array as well as Quantities. I think that's the best we can do now.

@@ -129,14 +128,9 @@ def __init__(self, lon_0, lat_0, sigma):
def evaluate(lon, lat, lon_0, lat_0, sigma):
"""Evaluate the model (static function)."""
sep = angular_separation(lon, lat, lon_0, lat_0)
sep = sep.to("rad").value
sigma = sigma.to("rad").value
norm = 1 / (2 * np.pi * sigma ** 2)

This comment has been minimized.

@registerrier

registerrier Nov 3, 2018

Contributor

How much slower is evaluation with quantities compared to regular floats?
Isn't it better to have the .to_value()at the beginning and just multiply with the correct unit at the end?

This comment has been minimized.

@adonath

adonath Nov 5, 2018

Member

I don't know how the performance compares, but the idea here is, that the .evaluate() method should avoid code that is specific to quantities as much as possible. This way we can globally (e.g. in the __call__ method) control, whether we use pure np.array or Quantity in the evaluation. I think that's the best we can do now.

npred = Map.from_geom(self.geom, unit="")
npred.data = (flux * self.exposure.quantity).to("").value
return npred
npred = (flux * self.exposure.quantity).to_value("")

This comment has been minimized.

@registerrier

registerrier Nov 3, 2018

Contributor

This can now be replaced with map arithmetics: npred = exposure * flux

This comment has been minimized.

@adonath

adonath Nov 5, 2018

Member

With map arithmetics there is an additional copy of the data involved, which I'd like to avoid here, because it is called many times during a fit. I'll keep as is for now.

energies = []
for val in np.atleast_1d(value):
def f(x):
# scale by 1e12 to achieve better precision
y = self(x * u.TeV).to(value.unit).value
energy = u.Quantity(x, eunit)

This comment has been minimized.

@registerrier

registerrier Nov 3, 2018

Contributor

why not copy=False here too?

This comment has been minimized.

@adonath
].quantity.to("TeV").value ** (-index + 1)
term = (bottom / top) * (value / p["amplitude"].quantity).to("1 / TeV")
return np.power(term.value, -1.0 / index) * u.TeV
bottom = emax - emin * (emin / emax) ** (-index)

This comment has been minimized.

@registerrier

registerrier Nov 3, 2018

Contributor

Same comment as above about evaluations with quantities.

@@ -254,7 +254,7 @@ class ImageProfile(object):
def __init__(self, table):
self.table = table
def smooth(self, kernel="box", radius=0.1 * u.deg, **kwargs):
def smooth(self, kernel="box", radius="0.1 deg", **kwargs):

This comment has been minimized.

@mackaiver

mackaiver Nov 5, 2018

Contributor

What happens if somebody tries to pass a quantity for the radius? does it still work as expected?

This comment has been minimized.

@adonath

adonath Nov 5, 2018

Member

Yes, passing in a Quantity still works the same way. The string argument just offers additional convenience to the user. Since Astropy 2.0 it is possible to instantiate quantities with strings, which I find nicer to use as creating the quantity with 0.1 * u.deg.

This comment has been minimized.

@mackaiver

mackaiver Nov 5, 2018

Contributor

👍

This comment has been minimized.

@mackaiver

mackaiver Nov 5, 2018

Contributor

We should update the dependencies in the setup.py to astropy 2.0 then.

This comment has been minimized.

@adonath

adonath Nov 5, 2018

Member

Done in this PR see 6813e08.

@adonath adonath force-pushed the adonath:review_quantity_handling branch from 0f4c794 to 9cdb749 Nov 5, 2018

@adonath adonath merged commit 089d552 into gammapy:master Nov 6, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 2568 new issues – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@adonath adonath deleted the adonath:review_quantity_handling branch Nov 6, 2018

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