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

Add another doctest to connecting conversion maps #12990

Closed
lftabera opened this issue May 22, 2012 · 15 comments
Closed

Add another doctest to connecting conversion maps #12990

lftabera opened this issue May 22, 2012 · 15 comments

Comments

@lftabera
Copy link
Contributor

between 4.7.x and 5.x series, the following error was introduced:

K = NumberField([x^2-2, x^2-3], 'a,b')
M = K.absolute_field('c')
M_to_K, K_to_M = M.structure()
M.register_coercion(K_to_M)
K.register_coercion(M_to_K)
M.coerce_map_from(QQ)
...
UnboundLocalError: local variable 'connecting' referenced before assignment

This error was fixed in #12919. We propose to add a new test to that fix.

CC: @simon-king-jena

Component: coercion

Author: Simon King, Luis Felipe Tabera Alonso

Reviewer: Keshav Kini, Marco Streng

Merged: sage-5.3.beta2

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

@lftabera
Copy link
Contributor Author

comment:1

The solution was found by Simon King discussing #12269

@lftabera
Copy link
Contributor Author

Author: Simon King, Luis Felipe Tabera Alonso

@nbruin
Copy link
Contributor

nbruin commented May 22, 2012

comment:3

OK, nevermind attachment: trac_12990.patch. Overlapping time windows! Just go with the first patch. The typo was introduced in #7420, which was merged in 4.3. Apparently changes after 4.7.1 made it so that the example in the ticket activates this code path.

@kini
Copy link
Collaborator

kini commented May 22, 2012

comment:4

patchbot: apply connecting.patch

@kini
Copy link
Collaborator

kini commented May 22, 2012

Reviewer: Keshav Kini

@kini
Copy link
Collaborator

kini commented May 22, 2012

comment:5

Looks good to go.

@jdemeyer
Copy link

comment:6

Looks like like a duplicate of #12919.

@kini
Copy link
Collaborator

kini commented May 26, 2012

comment:7

Well would you look at that... hmm. Maybe we can keep the doctest from this patch. I've posted a comment on #12919.

@lftabera
Copy link
Contributor Author

comment:8

Yes, whis is a clear duplicate of #12919, however, I would like to keep also the doctest of this patch, 12919 doctest is a an ad-hoc class for testing the bug. In this patch is a "real-life" situation where the bug is triggered.

Should I clear the patch to keep only the doctest and possibli be merged?

@kini
Copy link
Collaborator

kini commented May 31, 2012

comment:9

Sounds good to me.

@lftabera

This comment has been minimized.

@lftabera lftabera changed the title typo in connecting conversion maps Add another doctest to connecting conversion maps Jun 22, 2012
@mstreng
Copy link

mstreng commented Aug 6, 2012

Changed reviewer from Keshav Kini to Keshav Kini, Marco Streng

@mstreng
Copy link

mstreng commented Aug 6, 2012

comment:11

Attachment: connecting.patch.gz

@kini
Copy link
Collaborator

kini commented Aug 6, 2012

comment:12

Whoops, forgot about this ticket. Thanks for the positive review, mstreng.

@jdemeyer
Copy link

Merged: sage-5.3.beta2

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

6 participants