Opened 8 years ago
Closed 8 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 )
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)
Change History (21)
comment:1 Changed 8 years ago by
- Description modified (diff)
comment:2 Changed 8 years ago by
- 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
- 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
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
- 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
- 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
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
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
- 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
- Status changed from needs_review to positive_review
comment:11 Changed 8 years ago by
Let me add that this halved the running time of a program. Thanks!
comment:12 Changed 8 years ago by
- 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: ↓ 16 Changed 8 years ago by
- 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 8 years ago by
comment:14 follow-up: ↓ 15 Changed 8 years ago by
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 8 years ago by
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: ↓ 17 Changed 8 years ago by
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 8 years ago by
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()
orfree()
will likely crash Sage butsage_malloc()
and friends contain code to block interrupts.
OOOOOoooooooooohhhhhhhhh !!! Thanks !!
Nathann
comment:18 Changed 8 years ago by
- Status changed from needs_review to positive_review
This ticket works fine for me.
comment:19 Changed 8 years ago by
- Description modified (diff)
- Summary changed from copying a linear program crashes SAGE to copying a linear program crashes Sage
comment:20 Changed 8 years ago by
- Merged in set to sage-4.7.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
sorry -- forgot the code block markup...