Opened 9 years ago

Closed 9 years ago

#13119 closed defect (fixed)

._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@… Merged in: sage-5.6.beta2
Authors: Mike Zabrocki Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by tscrim)

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

Attachments (1)

trac_13119_apply_module_morphism_bugfix-mz.patch (3.8 KB) - added by zabrocki 9 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 9 years 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 9 years ago by zabrocki

  • Status changed from new to needs_review

comment:3 Changed 9 years ago by zabrocki

Removed a bit of whitespace within the function

Apply: trac_13119_apply_module_morphism_bugfix-mz.patch

Last edited 9 years ago by zabrocki (previous) (diff)

comment:4 Changed 9 years ago by tscrim

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

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

comment:5 Changed 9 years 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 9 years ago by tscrim

  • Status changed from needs_review to positive_review

Looks good. Thanks Mike.

comment:7 Changed 9 years ago by jdemeyer

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