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

Better interaction between function units and equivalencies #5017

Merged
merged 4 commits into from
Jun 17, 2016

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Jun 2, 2016

An alternative solution to #5012 that ensures that magnitudes and other functional units can be converted to even if also an equivalency is needed. I think it is a better approach, being more general, but it does leave a few questions:

  1. Would this be better implemented by letting non-units have the ability to convert from a unit (rather than just to)? Or perhaps have a _get_converter staticmethod that allows one to give the units in arbitrary order.
  2. Would it be better to have a general ability to chain equivalencies (as is effectively done here).

My sense would be to leave these questions for the future (can raise a separate issue), and just merge this as this is simple and just extends what worked already in a logical fashion.

With this PR, the following works; @pllim - is this indeed good enough for you?

In [2]: t = 1.*u.erg/u.s/u.cm**2/u.AA

In [3]: u.add_enabled_equivalencies(u.spectral_density(5500*u.AA))
Out[3]: <astropy.units.core._UnitContext at 0x7f5caf5a3f60>

In [4]: t.to(u.ABmag)
Out[4]: <Quantity -21.109761690151405 mag(AB)>

@pllim
Copy link
Member

pllim commented Jun 2, 2016

Thanks! I'll take this for a spin tomorrow.

@@ -313,6 +313,9 @@ New Features
- Add bolometric absolute and apparent magnitudes, ``M_bol`` and ``m_bol``.
[#4262]

- Ensured that with ``spectral_density`` equivalency one could also convert
Copy link
Member

Choose a reason for hiding this comment

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

You marked the PR as a bug fix but the entry is under "new features". Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, just a silly mistake to have it under new features; now corrected (I also rebased so there is no new commit)

@pllim
Copy link
Member

pllim commented Jun 3, 2016

This looks good and more elegant than my solution, so I am closing #5012 in favor of this as well. Thank you!

One thing I stumbled upon while checking the results is that assert_allclose cannot handle the mag units (see below). Not sure if it is a bug or not.

>>> y
<Quantity [ 12.41858666, 12.38919214, 12.41764369] mag(ST)>
>>> _flux_stmag
<Quantity [ 12.41858665, 12.38919182, 12.41764379] mag(ST)>
>>> np.testing.assert_allclose(y, _flux_stmag)
.../numpy/testing/utils.pyc in assert_allclose(actual, desired, rtol, atol, equal_nan, err_msg, verbose)
   1357     header = 'Not equal to tolerance rtol=%g, atol=%g' % (rtol, atol)
   1358     assert_array_compare(compare, actual, desired, err_msg=str(err_msg),
-> 1359                          verbose=verbose, header=header)
   1360 
   1361 def assert_array_almost_equal_nulp(x, y, nulp=1):

.../numpy/testing/utils.pyc in assert_array_compare(comparison, x, y, err_msg, verbose, header, precision)
    692                 val = safe_comparison(x[~x_id], y[~y_id])
    693             else:
--> 694                 val = safe_comparison(x, y)
    695         else:
    696             val = safe_comparison(x, y)

.../numpy/testing/utils.pyc in safe_comparison(*args, **kwargs)
    639         with warnings.catch_warnings():
    640             warnings.filterwarnings("ignore", category=DeprecationWarning)
--> 641             return comparison(*args, **kwargs)
    642 
    643     def isnumber(x):

.../numpy/testing/utils.pyc in compare(x, y)
   1352     def compare(x, y):
   1353         return np.core.numeric.isclose(x, y, rtol=rtol, atol=atol,
-> 1354                                        equal_nan=equal_nan)
   1355 
   1356     actual, desired = np.asanyarray(actual), np.asanyarray(desired)

.../numpy/core/numeric.pyc in isclose(a, b, rtol, atol, equal_nan)
   2357     yfin = isfinite(y)
   2358     if all(xfin) and all(yfin):
-> 2359         return within_tol(x, y, atol, rtol)
   2360     else:
   2361         finite = xfin & yfin

.../numpy/core/numeric.pyc in within_tol(x, y, atol, rtol)
   2340     def within_tol(x, y, atol, rtol):
   2341         with errstate(invalid='ignore'):
-> 2342             result = less_equal(abs(x-y), atol + rtol * abs(y))
   2343         if isscalar(a) and isscalar(b):
   2344             result = bool(result)

.../astropy/units/quantity.pyc in __rmul__(self, other)
    829 
--> 830         return self.__mul__(other)
    831 
    832     def __truediv__(self, other):

.../astropy/units/quantity.pyc in __mul__(self, other)
    812                 return NotImplemented
    813 
--> 814         return super(Quantity, self).__mul__(other)
    815 
    816     def __imul__(self, other):

.../astropy/units/quantity.pyc in __array_prepare__(self, obj, context)
    321         # the unit the output from the ufunc will have.
    322         if function in UFUNC_HELPERS:
--> 323             converters, result_unit = UFUNC_HELPERS[function](function, *units)
    324         else:
    325             raise TypeError("Unknown ufunc {0}.  Please raise issue on "

.../astropy/units/quantity_helper.pyc in <lambda>(f, unit1, unit2)
    198 
    199 UFUNC_HELPERS[np.multiply] = lambda f, unit1, unit2: (
--> 200     [None, None], _d(unit1) * _d(unit2))
    201 
    202 helper_division = lambda f, unit1, unit2: ([None, None], _d(unit1) / _d(unit2))

.../astropy/units/function/core.pyc in __mul__(self, other)
    297                 return self.function_unit * other
    298             else:
--> 299                 raise UnitsError("Cannot multiply a function unit "
    300                                  "with a physical dimension with any unit.")
    301         else:

UnitsError: Cannot multiply a function unit with a physical dimension with any unit.

@mhvk
Copy link
Contributor Author

mhvk commented Jun 4, 2016

numpy's assert_allclose does not work well with Quantity generally (in that it ignores the unit); do use tests.helper.assert_quantity_allclose.

@mhvk mhvk force-pushed the function-units-and-equivalencies branch from 776dd99 to fef1f1b Compare June 4, 2016 16:39
@mhvk mhvk force-pushed the function-units-and-equivalencies branch from fef1f1b to c43cb67 Compare June 4, 2016 16:41
@mhvk
Copy link
Contributor Author

mhvk commented Jun 6, 2016

@astrofrog - would you be able to review this? I think it is fine, although slightly hacky (but not much more than it was before).

@astrofrog
Copy link
Member

This looks good to me - sorry for not merging it before.

@astrofrog astrofrog merged commit a46bd0b into astropy:master Jun 17, 2016
@mhvk mhvk deleted the function-units-and-equivalencies branch June 17, 2016 13:10
astrofrog added a commit that referenced this pull request Jun 18, 2016
Better interaction between function units and equivalencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants