Opened 7 years ago

Closed 7 years ago

#12990 closed defect (fixed)

Add another doctest to connecting conversion maps

Reported by: lftabera Owned by: robertwb
Priority: major Milestone: sage-5.3
Component: coercion Keywords:
Cc: SimonKing Merged in: sage-5.3.beta2
Authors: Simon King, Luis Felipe Tabera Alonso Reviewers: Keshav Kini, Marco Streng
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by lftabera)

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.

Attachments (1)

connecting.patch (1.1 KB) - added by lftabera 7 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by lftabera

  • Authors set to Simon King, Luis Felipe Tabera Alonso
  • Cc SimonKing added

The solution was found by Simon King discussing #12269

comment:2 Changed 7 years ago by lftabera

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by nbruin

OK, nevermind 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.

comment:4 Changed 7 years ago by kini

patchbot: apply connecting.patch

comment:5 Changed 7 years ago by kini

  • Reviewers set to Keshav Kini
  • Status changed from needs_review to positive_review

Looks good to go.

comment:6 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Looks like like a duplicate of #12919.

comment:7 Changed 7 years ago by kini

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

comment:8 Changed 7 years ago by lftabera

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?

comment:9 Changed 7 years ago by kini

Sounds good to me.

comment:10 Changed 7 years ago by lftabera

  • Description modified (diff)
  • Status changed from needs_info to needs_review
  • Summary changed from typo in connecting conversion maps to Add another doctest to connecting conversion maps

Changed 7 years ago by lftabera

comment:11 Changed 7 years ago by mstreng

  • Reviewers changed from Keshav Kini to Keshav Kini, Marco Streng
  • Status changed from needs_review to positive_review

comment:12 Changed 7 years ago by kini

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

comment:13 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.3.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.