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

Fix unit tests errors with numpy >=1.13 (Fix 577) #630

Merged
merged 7 commits into from
Apr 13, 2018

Conversation

cpascual
Copy link
Contributor

This PR fixes the 4 test errors/failiures reported in #577 and adds new travis builds with numpy 1.14

3 of the 4 problems are relatively trivial to solve.

The remaining one (test_isreal) is not so trivial because it involves allowing to compare a non-dimensionless quantity against zero. The reason is that numpy.isreal(x) essentially does x.imag !=0 if the imag method is implemented in x... and since Quantity reimplements .imag to return units (see #171), the check always returns False if x is a non-dimensionless quantity.

As mentioned, this can be solved by allowing comparisons against zero. Actually, this has already been requested in #194, but it was not implemented.

In this PR, I implemented the comparison-to-zero feature and provide several new unit tests for it. AFAIK this implementation takes into account the concerns expressed by @hgrecco in #194. But if the feature is not wanted, we can just revert the last 3 commits of this PR and then test_isreal will left with an expected failure flag.

The rules that I implemented for the comparison-with-zero are:

    a) comparing against (non-quantity) zero is allowed for any quantity
    b) comparing against zero-magnitude quantities of incompatible
    dimensionality raises a Dimensionality error, except for the case
    of equality, for which it always returns False
    
    Notes:
    1.- Numpy arrays of zeros are also supported and the comparison rules
    apply elementwise
    2.- In the case of non-multiplicative units, the rules above
    apply after converting to base units if the autoconvert offset flag
    is set. Otherwise they raise an OffsetUnitCalculusError.

Fixes #577

Carlos Pascual added 7 commits April 13, 2018 19:07
Test pint in travis against the latest numpy (currently 1.14.x)
In nummpy >1.11, the a**=b raises an exception if a and b are quantities
and a.magnitude is a ndarray.

Fix this case
pint.testsuite.test_umath.TestFloatingUfuncs.test_isfinite uses
np.isreal instead of np.isfinite. This is probably a copy-paste error.
Fix it
np.isreal(v) is implemented (at least in np >=1.13) as a check that
v.imag != 0 . Since Quantity reimplements imag to return units (see
issue hgrecco#171) , np.isreal always returns False for any quantity.
Therefore, this test is marked as expected to fail for now (note that
the tests with np <1.13 will now yield an unexpected success).
Add a test that checks various cases of comparing a quantity to
(non-quantity) zero.
Comparisons against zero should work even for non-dimensionless
quantities, and non-multiplicative quantities should be properly
supported.
Implement the following special behaviour when comparing a quantity
against zero:

a) comparing against (non-quantity) zero is allowed for any quantity
b) comparing against zero-magnitude quantities of incompatible
dimensionality raises a Dimensionality error, except for the case
of equality, for which it always returns False

Notes:
1.- Numpy arrays of zeros are also supported and the comparison rules
apply elementwise
2.- In the case of non-multiplicative units, the rules above
apply after converting to base units if the autoconvert offset flag
is set. Otherwise they raise an OffsetUnitCalculusError.
This reverts commit fcbceaef41b278fd57c7bab7dc901f35de8c154d.
@cpascual
Copy link
Contributor Author

Hi @hgrecco , as we talked in #614, I would be happy of practising the bors workflow with this PR.
But I do not think that I should merge my own PR until someone else (you or someone else with commit rights) gives me a green light. So, would you have a look or appoint someone else to do it?

@cpascual
Copy link
Contributor Author

bors try

@bors
Copy link
Contributor

bors bot commented Apr 13, 2018

🔒 Permission denied

Existing reviewers: click here to make cpascual a reviewer

@cpascual
Copy link
Contributor Author

bors try

It seems that @hgrecco should give me permissions in bors beforeI can try... I got this email:

:lock: Permission denied

Existing reviewers: [click here to make cpascual a reviewer](https://app.bors.tech/repositories/2346/add-reviewer/cpascual)

@hgrecco
Copy link
Owner

hgrecco commented Apr 13, 2018

Great work. Go ahead and try to merge it (I could do it but I want to try again the Bors workflow, I have given you access).

@cpascual
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Apr 13, 2018
@bors
Copy link
Contributor

bors bot commented Apr 13, 2018

try

Build succeeded

@cpascual
Copy link
Contributor Author

cpascual commented Apr 13, 2018

I attempted to do a bors try and the outcome is kind of confusing to me: a new travis test appeared which is failing due to what it looks like a bors/github configuration problem (fatal: Remote branch trying.tmp not found in upstream origin).

So now I am going to attempt the bors r+ directly

@cpascual
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Apr 13, 2018
630: Fix unit tests errors with numpy >=1.13 (Fix 577) r=cpascual a=cpascual

This PR fixes the 4 test errors/failiures reported in #577 and adds new travis builds with numpy 1.14

3 of the 4 problems are relatively trivial to solve.

The remaining one (`test_isreal`) is not so trivial because it  involves allowing to compare a *non-dimensionless* quantity against zero. The reason is that `numpy.isreal(x)` essentially does `x.imag !=0` if the `imag` method is implemented in x... and since Quantity reimplements .imag to return units (see #171), the check always returns False if x is a non-dimensionless quantity.

As mentioned, this can be solved by allowing comparisons against zero. Actually, this has already been requested in #194, but it was not implemented.

In this PR, I implemented the comparison-to-zero feature and provide several new unit tests for it. AFAIK this implementation takes into account the concerns expressed by @hgrecco in #194. But if the feature is not wanted, we can just revert the last 3 commits of this PR and then `test_isreal` will left with an expected failure flag.

The rules that I implemented for the comparison-with-zero are:
```    
    a) comparing against (non-quantity) zero is allowed for any quantity
    b) comparing against zero-magnitude quantities of incompatible
    dimensionality raises a Dimensionality error, except for the case
    of equality, for which it always returns False
    
    Notes:
    1.- Numpy arrays of zeros are also supported and the comparison rules
    apply elementwise
    2.- In the case of non-multiplicative units, the rules above
    apply after converting to base units if the autoconvert offset flag
    is set. Otherwise they raise an OffsetUnitCalculusError.
```

Fixes #577


Co-authored-by: Carlos Pascual <cpascual@cells.es>
@bors
Copy link
Contributor

bors bot commented Apr 13, 2018

Build succeeded

@bors bors bot merged commit 4f50583 into hgrecco:master Apr 13, 2018
@cpascual cpascual deleted the fix-577 branch April 13, 2018 23:12
@cpascual
Copy link
Contributor Author

cpascual commented Apr 13, 2018

Since this was an experiment on usin bors, here are some observations:

  • even if bors r+ succeeded, I see that each invocation triggers 2 builds in travis, one on the staging.tmp branch and another on the staging branch. Of these, the first systematically fails with what looks like a config problem. But the actual result of the command seems to be affected only by the result of the second.

  • a similar thing happens with the bors try command

In the bors docs it says:

Your CI system should build the “staging” and “trying” branches, but should not build a branch called “staging.tmp”

I think that this can be fixed by adding the following blacklist to travis.yml (I submitted PR #631 to test it)

branches:
  except:
  - staging.tmp
  - trying.tmp

bors bot added a commit that referenced this pull request Apr 14, 2018
631: prevent Travis from building bors tmp branches r=cpascual a=cpascual

As noticed in #630 (comment) , Travis should not build the staging.tmp and trying.tmp branches used internally by bors. So blocklist them in travis.yml

Co-authored-by: Carlos Pascual <cpascual@cells.es>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet