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

test_simulation.py fails with the latest version of numpy #478

Closed
mishinma opened this issue Feb 7, 2016 · 8 comments
Closed

test_simulation.py fails with the latest version of numpy #478

mishinma opened this issue Feb 7, 2016 · 8 comments

Comments

@mishinma
Copy link
Contributor

mishinma commented Feb 7, 2016

@yeganer @unoebauer @wkerzendorf
In PR #471 the following (a few of them actually) asset statement was added to test_simulation.py:

npt.assert_allclose(
            t_rad, simulation_compare_data['test1/t_rad'], atol=0.0)

It is calculated like this: absolute(a - b) <= (atol + rtol * absolute(b))
The problem here is that t_rad is a Quantity while simulation_compare_data['test1/t_rad'] is just a numpy array. And if somehow works with numpy v 1.9.0, with v 1.10 it fails. An error is raised in the __array_prepare__ method that is overridden in Astropy:


try:
                if _can_have_arbitrary_unit(maybe_arbitrary_arg):
                    converters = [None, None]
                else:
                    raise UnitsError("Can only apply '{0}' function to "
                                     "dimensionless quantities when other "
                                     "argument is not a quantity (unless the "
                                     "latter is all zero/infinity/nan)"
>                                    .format(function.__name__))
E                                    UnitsError: Can only apply 'subtract' function to dimensionless quantities when other argument is not a quantity (unless the latter is all zero/infinity/nan)
@mishinma
Copy link
Contributor Author

mishinma commented Feb 7, 2016

Actually, it would be nice to compare Quantities with a special util function that would validate the units of its arguments. One version of such a function is implemented in Astropy's module tests/helpers.py: assert_quantity_allclose

@yeganer
Copy link
Contributor

yeganer commented Feb 7, 2016

I think the right approach should be to use astropy's assert_quantity_allclose. One would have to test if that solution is compatible with different versions of numpy and astropy.

@mishinma
Copy link
Contributor Author

mishinma commented Feb 7, 2016

But units still should be added to the tests. assert_quantity_allclose still raises an error in this case as well, although with meaningful message:

(Pdb) assert_quantity_allclose(t_rad, simulation_compare_data['test1/t_rad'], atol=0.0)
*** UnitsError: Units for 'desired' () and 'actual' (K) are not convertible
(Pdb) npt.assert_allclose(t_rad, simulation_compare_data['test1/t_rad'], atol=0.0)
*** UnitsError: Can only apply 'subtract' function to dimensionless quantities when other argument is not a quantity (unless the latter is all zero/infinity/nan)

@yeganer
Copy link
Contributor

yeganer commented Feb 8, 2016

So we have two options. Strip the result of its unit or add the unit to the
compare data. In that case we should do the second option.

Misha [email protected] schrieb am So., 7. Feb. 2016 21:08:

But units still should be added to the tests. assert_quantity_allclose
still raises an error in this case as well, although with more clear
message:

(Pdb) assert_quantity_allclose(t_rad, simulation_compare_data['test1/t_rad'], atol=0.0)
*** UnitsError: Units for 'desired' () and 'actual' (K) are not convertible
(Pdb) npt.assert_allclose(t_rad, simulation_compare_data['test1/t_rad'], atol=0.0)
*** UnitsError: Can only apply 'subtract' function to dimensionless quantities when other argument is not a quantity (unless the latter is all zero/infinity/nan)


Reply to this email directly or view it on GitHub
#478 (comment).

@unoebauer unoebauer added this to the v1.5 milestone Feb 8, 2016
@yeganer
Copy link
Contributor

yeganer commented Feb 8, 2016

Thank you for pointing out numpy's assert_quantity_allclose. I think we should use it whenever possible in our tests.

@wkerzendorf @unoebauer What do you think?

@mishinma
Copy link
Contributor Author

mishinma commented Feb 8, 2016

I made PR #479 to use assert_quantity_allclose in test_simulation.py

@mishinma
Copy link
Contributor Author

mishinma commented Feb 9, 2016

Should the issue be closed now?

@unoebauer
Copy link
Contributor

Yes - closing it now.

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

No branches or pull requests

3 participants