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

meataxe doctest failure on 8.2.beta8 #24947

Closed
videlec opened this issue Mar 11, 2018 · 13 comments
Closed

meataxe doctest failure on 8.2.beta8 #24947

videlec opened this issue Mar 11, 2018 · 13 comments

Comments

@videlec
Copy link
Contributor

videlec commented Mar 11, 2018

Mainly because the constructor of matrices changed

      File "sage/matrix/matrix_gfpn_dense.pyx", line 304, in sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense.__init__ (build/cythonized/sage/matrix/matrix_gfpn_dense.c:4524)
        def __init__(self, parent, entries=None, *, bint copy=False, bint coerce=False, bint mutable=True):
    TypeError: __init__() takes at most 2 positional arguments (4 given)

See quasar short log

CC: @jdemeyer @simon-king-jena

Component: linear algebra

Author: Jeroen Demeyer

Branch/Commit: 447b4e5

Reviewer: Simon King

Issue created by migration from https://trac.sagemath.org/ticket/24947

@videlec videlec added this to the sage-8.2 milestone Mar 11, 2018
@videlec videlec changed the title meataxe failing on 8.2.beta8 meataxe doctest failure on 8.2.beta8 Mar 11, 2018
@simon-king-jena
Copy link
Member

comment:2

I guess that's the change in matrix initialisation introduced by Jeroen. But hasn't that been tested with optional matrix backends?

@videlec
Copy link
Contributor Author

videlec commented Mar 11, 2018

comment:3

Replying to @simon-king-jena:

I guess that's the change in matrix initialisation introduced by Jeroen. But hasn't that been tested with optional matrix backends?

I just did :-) One problem is that on the previous beta, there was a constant failure on patchbots (#24918). Hence the patchbot quasar (running with all optional packages) was off. Until everybody agrees that each beta must pass with all optional packages you will have to wait (see how much do we support optional packages and #23832).

@jdemeyer
Copy link

comment:4

Sorry for that...

This is actually fixed in #24742. Should I create a separate fix for just this issue or does anybody plan to review #24742?

@videlec
Copy link
Contributor Author

videlec commented Mar 11, 2018

comment:5

Replying to @jdemeyer:

Sorry for that...

This is actually fixed in #24742. Should I create a separate fix for just this issue or does anybody plan to review #24742?

Better to have a fix in an independent ticket (this one). #24742 is likely to take more time.

@jdemeyer
Copy link

Author: Jeroen Demeyer

@jdemeyer
Copy link

@videlec
Copy link
Contributor Author

videlec commented Mar 11, 2018

comment:8

Replying to @videlec:

Replying to @simon-king-jena:

I guess that's the change in matrix initialisation introduced by Jeroen. But hasn't that been tested with optional matrix backends?

I just did :-) One problem is that on the previous beta, there was a constant failure on patchbots (#24918). Hence the patchbot quasar (running with all optional packages) was off. Until everybody agrees that each beta must pass with all optional packages you will have to wait (see how much do we support optional packages and #23832).

Actually #24918 is not merged in the new beta so that the patchbots are still useless.


New commits:

447b4e5Properly fix signature of Matrix_gfpn_dense.__init__

@videlec
Copy link
Contributor Author

videlec commented Mar 11, 2018

Commit: 447b4e5

@jdemeyer
Copy link

comment:10

Interestingly, this failure is caused by the combination of two merged tickets, each of which individually passed the testsuite. This is just to say that even a perfect patchbot cannot catch everything.

@jdemeyer
Copy link

comment:11

ping

@simon-king-jena
Copy link
Member

comment:12

If I understand correctly, you added your commit on 11th of March, but the patchbot gets errors on 12th of March.

However, both tests failures that I see are in test(interacts.statistics.coin), which apparently is unrelated with this ticket. And the commit looks good to me.

@simon-king-jena
Copy link
Member

Reviewer: Simon King

@vbraun
Copy link
Member

vbraun commented Mar 21, 2018

Changed branch from u/jdemeyer/meataxe_doctest_failure_on_8_2_beta8 to 447b4e5

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

4 participants