Opened 8 years ago

Closed 7 years ago

#11588 closed defect (fixed)

copying a linear program crashes Sage

Reported by: john_perry Owned by: ncohen
Priority: major Milestone: sage-4.7.2
Component: linear programming Keywords: MixedIntegerLinearProgram, copy
Cc: Merged in: sage-4.7.2.alpha3
Authors: Nathann Cohen Reviewers: John Perry
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

It would be useful to copy a linear program, so that one doesn't have to recopy the constraints. Unfortunately, this crashes Sage, and pretty hard.

sage: eqs = MixedIntegerLinearProgram()
sage: eqs2 = copy(eqs)
sage: eqs2

------------------------------------------------------------
Unhandled SIGSEGV: A segmentation fault occurred in Sage.
This probably occurred because a *compiled* component
of Sage has a bug in it (typically accessing invalid memory)
and is not properly wrapped with sig_on(), sig_off().
You might want to run Sage under gdb with 'sage -gdb' to debug this.
Sage will now terminate (sorry).
------------------------------------------------------------

Apply trac_11588.patch to the Sage library.

Attachments (1)

trac_11588.patch (9.2 KB) - added by ncohen 7 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by john_perry

  • Description modified (diff)

sorry -- forgot the code block markup...

comment:2 Changed 8 years ago by ncohen

  • Authors set to Nathann Cohen
  • Status changed from new to needs_review

I curse the day I wrote this Coin Branch and Cut support in Sage :-D

But all in all, I got through.... Here's a patch ready for review ! Please test it extensively, as I do not know why you need to copy them for I probably did not think of enough use cases :-)

Neeeeeeeeds revieeeeeeeeeww !!!

Nathann

comment:3 Changed 8 years ago by john_perry

  • Status changed from needs_review to needs_work

Copying works only if there are no constraints... that's said, it's not clear to me where the error here lies.

sage: lp = MixedIntegerLinearProgram()
sage: lp.add_constraint(lp[0]-lp[1],min=1)
sage: lp.add_constraint(2*lp[1] - lp[0] - lp[2],min=1)
sage: np = copy(lp)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/atlas/perry/<ipython console> in <module>()

/usr/local/share/sage-4.7-linux-32bit-ubuntu_10.04_lts-i686-Linux/local/lib/python/copy.pyc in copy(x)
     77     copier = getattr(cls, "__copy__", None)
     78     if copier:
---> 79         return copier(x)
     80 
     81     reductor = dispatch_table.get(cls)

/usr/local/share/sage-4.7-linux-32bit-ubuntu_10.04_lts-i686-Linux/local/lib/python2.6/site-packages/sage/numerical/mip.so in sage.numerical.mip.MixedIntegerLinearProgram.__copy__ (sage/numerical/mip.c:1859)()

/usr/local/share/sage-4.7-linux-32bit-ubuntu_10.04_lts-i686-Linux/local/lib/python/copy.pyc in copy(x)
     93                 raise Error("un(shallow)copyable object of type %s" % cls)
     94 
---> 95     return _reconstruct(x, rv, 0)
     96 
     97 

/usr/local/share/sage-4.7-linux-32bit-ubuntu_10.04_lts-i686-Linux/local/lib/python/copy.pyc in _reconstruct(x, info, deep, memo)
    321     if deep:
    322         args = deepcopy(args, memo)
--> 323     y = callable(*args)
    324     memo[id(x)] = y
    325     if listiter is not None:

/usr/local/share/sage-4.7-linux-32bit-ubuntu_10.04_lts-i686-Linux/local/lib/python/copy_reg.pyc in __newobj__(cls, *args)
     91 
     92 def __newobj__(cls, *args):
---> 93     return cls.__new__(cls, *args)
     94 
     95 def _slotnames(cls):

/usr/local/share/sage-4.7-linux-32bit-ubuntu_10.04_lts-i686-Linux/local/lib/python2.6/site-packages/sage/numerical/mip.so in sage.numerical.mip.MIPVariable.__cinit__ (sage/numerical/mip.c:6626)()

TypeError: __cinit__() takes at least 2 positional arguments (0 given)

comment:4 Changed 8 years ago by john_perry

Incidentally, would it be possible also to add the functionality of deleting constraints? or should I open a different ticket for that?

comment:5 Changed 8 years ago by ncohen

  • Status changed from needs_work to needs_review

Yo !

It actually only failed when using the lp[x] variables instead of creating new variables and using the to define the constraints ;-)

Patch updated.

Incidentally, would it be possible also to add the functionality of deleting constraints? or should I open a different ticket for that?

It is better to create a new ticket as this one already has a patch and fixes a nasty bug.

Nathann

comment:6 Changed 8 years ago by john_perry

  • Owner changed from ncohen to (none)

Did you provide the correct attachment? It has the same filename, and I'm getting the same error.

comment:7 Changed 8 years ago by ncohen

Yop !

It is indeed the new file. The difference with the former file is the line :

p._default_mipvariable = self._default_mipvariable 

Nathann

comment:8 Changed 8 years ago by john_perry

Okay, I'll try it again. I think I wrecked my sage installation anyway, so I'm reinstalling it.

BTW, I don't know how the owner was changed, and I can't figure out how it change it back to you...

comment:9 Changed 8 years ago by john_perry

  • Owner changed from (none) to ncohen

I don't know what I was doing wrong there, but it looks good now.

comment:10 Changed 8 years ago by john_perry

  • Status changed from needs_review to positive_review

comment:11 Changed 8 years ago by john_perry

Let me add that this halved the running time of a program. Thanks!

comment:12 Changed 8 years ago by jdemeyer

  • Reviewers set to John Perry
  • Status changed from positive_review to needs_work

Instead of free() you should use sage_free(), just like you use sage_malloc().

comment:13 follow-up: Changed 7 years ago by ncohen

  • Status changed from needs_work to needs_review

oops ! Sorry for that :-)

It turns out that changing this method was not a good idea though. Avoiding to use the C++ class was intended as a way to save on unseless computations, but it looks like the Coin library transforms the arrays into the same structure immediately when called, so everything is done twice, so I removed that part :-)

By the way, do you know what is the difference between sage_free and free, or between sage_malloc and malloc ? I always wondered :-)

Nathann

Changed 7 years ago by ncohen

comment:14 follow-up: Changed 7 years ago by john_perry

Sorry I didn't answer sooner; I thought jdemeyer would. I don't see any instance of either free() or sage_free() in the patch, so I don't know whether to mark it as positive_review. If I understand the conversation correctly, though, the original patch contained some free()'s that were not needed, but those have now been removed?

comment:15 in reply to: ↑ 14 Changed 7 years ago by ncohen

Replying to john_perry:

Sorry I didn't answer sooner; I thought jdemeyer would. I don't see any instance of either free() or sage_free() in the patch, so I don't know whether to mark it as positive_review. If I understand the conversation correctly, though, the original patch contained some free()'s that were not needed, but those have now been removed?

Precisely :-)

Nathann

comment:16 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by jdemeyer

Replying to ncohen:

By the way, do you know what is the difference between sage_free and free, or between sage_malloc and malloc?

Interrupts. An interrupt during malloc() or free() will likely crash Sage but sage_malloc() and friends contain code to block interrupts.

comment:17 in reply to: ↑ 16 Changed 7 years ago by ncohen

Replying to jdemeyer:

Replying to ncohen:

By the way, do you know what is the difference between sage_free and free, or between sage_malloc and malloc?

Interrupts. An interrupt during malloc() or free() will likely crash Sage but sage_malloc() and friends contain code to block interrupts.

OOOOOoooooooooohhhhhhhhh !!! Thanks !!

Nathann

comment:18 Changed 7 years ago by john_perry

  • Status changed from needs_review to positive_review

This ticket works fine for me.

comment:19 Changed 7 years ago by leif

  • Description modified (diff)
  • Summary changed from copying a linear program crashes SAGE to copying a linear program crashes Sage

comment:20 Changed 7 years ago by leif

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