Ticket #13119 (closed defect: fixed)

Opened 11 months ago

Last modified 5 months ago

._apply_module_morphism() in combinat/free_module.py doesn't handle zero element properly

Reported by: zabrocki Owned by: sage-combinat
Priority: minor Milestone: sage-5.6
Component: combinatorics Keywords:
Cc: saliola@… Work issues:
Report Upstream: N/A Reviewers: Travis Scrimshaw
Authors: Mike Zabrocki Merged in: sage-5.6.beta2
Dependencies: Stopgaps:

Description (last modified by tscrim) (diff)

a potential defect in ._apply_module_morphism function calls list( self.basis().keys() ) when self.is_zero() is True. This works fine if self.basis() is a finite set, but causes a problem if self.basis() is an infinite set with an iteration function. In this case, the function hangs trying to list the infinite set.


Apply only: trac_13119_apply_module_morphism_bugfix-mz.patch Download

Attachments

trac_13119_apply_module_morphism_bugfix-mz.patch Download (3.8 KB) - added by zabrocki 5 months ago.

Change History

comment:1 Changed 6 months ago by zabrocki

I added a patch to this ticket. Discussion is at  https://groups.google.com/d/topic/sage-combinat-devel/yPjfNINtVbk/discussion

I changed one thing in addition since I had originally reported this bug. That is, if the codomain is not known and it looks for the parent of an element of the image to determine the codomain then raise a value error if the element returned by the function doesn't have a parent.

comment:2 Changed 6 months ago by zabrocki

  • Status changed from new to needs_review

comment:3 Changed 6 months ago by zabrocki

Removed a bit of whitespace within the function

Apply: trac_13119_apply_module_morphism_bugfix-mz.patch

Last edited 6 months ago by zabrocki (previous) (diff)

comment:4 Changed 5 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Description modified (diff)

Hey Mike,

Two minor things:

  • Could you use the auto-linking :func:`on_basis` in the docstring?
  • Since at some point, there is a shift to Python 3, I think we should start using that syntax:
    raise ValueError('Codomain could not be determined')
    

Looks good to me otherwise.

Thanks,
Travis

For patchbot:

Apply only: trac_13119_apply_module_morphism_bugfix-mz.patch

Changed 5 months ago by zabrocki

comment:5 Changed 5 months ago by zabrocki

Hi Travis, Thanks for picking this up. I made your changes and caught one or two more fixes in the docstring. -Mike

comment:6 Changed 5 months ago by tscrim

  • Status changed from needs_review to positive_review

Looks good. Thanks Mike.

comment:7 Changed 5 months ago by jdemeyer

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