Opened 10 years ago
Closed 10 years ago
#13807 closed defect (fixed)
Fix remaining temporary filename issues (like in animate.py)
Reported by: | Karl-Dieter Crisman | Owned by: | Jason Grout |
---|---|---|---|
Priority: | blocker | Milestone: | sage-5.5 |
Component: | misc | Keywords: | beginner |
Cc: | Merged in: | sage-5.5.rc1 | |
Authors: | John Palmieri | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
In #13579, for security reasons and modularity, we moved temporary filename constructors from sage.misc.misc
to their own module. Most things were moved with it, but a couple places in animate.py were missed. Let's make sure that all such things, including these, are fixed in this ticket.
See this ask.sagemath.org question for a real-life bug this caused.
Attachments (1)
Change History (12)
comment:1 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 10 years ago by
Keywords: | beginner added |
---|---|
Priority: | critical → blocker |
comment:3 Changed 10 years ago by
Milestone: | sage-5.6 → sage-5.5 |
---|
comment:4 follow-up: 5 Changed 10 years ago by
Authors: | → John Palmieri |
---|---|
Status: | new → needs_review |
The file sage/misc/misc.py has a line
from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles
so I think that sage.misc.misc.tmp_filename
should work, and the same for tmp_dir
. For example, coding/linear_code.py
has a function code2leon
with the lines
from sage.misc.misc import tmp_filename ... file_loc = tmp_filename()
and it seems to work fine. So I think we only need to fix the graphics_filename
issue. One way to do it:
-
sage/misc/misc.py
diff --git a/sage/misc/misc.py b/sage/misc/misc.py
a b import operator, os, stat, socket, sys, 39 39 import sage.misc.prandom as random 40 40 from lazy_string import lazy_string 41 41 42 from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles 42 from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles, graphics_filename 43 43 44 44 from banner import version, banner 45 45
But it's also not much harder (I think) to fix each individual use of graphics_filename
. See attached patch.
comment:5 Changed 10 years ago by
The file sage/misc/misc.py has a line
from sage.misc.temporary_file import tmp_dir, tmp_filename, delete_tmpfiles
I was wondering about this, but since this was broken I figured it was all broken... sorry for that wasted time.
But it's also not much harder (I think) to fix each individual use of
graphics_filename
. See attached patch.
Agreed. This patch is fine, modulo the next comment.
This could have been caught if we had a way to enable this currently untested doctest. The same problem in the matrix file - it's precisely when we don't pass in the filename that this branch of the code happens... sigh. Of course, that function is completely untested.
Can you think of any way to get around that and add a doctest to this patch, or is it hopeless without doing some pointless refactoring of code? William is very clear about not wanting to leave random files around... maybe we could at least doctest the matrix one and then remove that file? We've tried to get away from ad hoc file creation and removal, but in this case I think that's the only way to do it right.
Incidentally, ran into the sage -n
not starting while testing this... so I'm glad that will be fixed before the final 5.5, right?
Changed 10 years ago by
Attachment: | trac_13807_graphics_filename.patch added |
---|
comment:6 follow-up: 7 Changed 10 years ago by
comment:7 Changed 10 years ago by
I've added a doctest (not a perfect one, but the best I can think of) in matrix_modn_sparse.pyx.
This makes sense, but unfortunately it doesn't actually show up in the notebook. In fact, even at the command line it shows up in the current working directory, which is not standard Sage behavior for graphics, though it is the documented behavior. My personal preference would be to have it changed, but that wouldn't be this ticket. What do you think the "right" behavior should be for the notebook and EMBEDDED_MODE
? Not that I'm angling for a line for that situation, but it would be nice to have.
Actually, this must be a really old function, since matrix_plot
does what we want here. hg blame
tells me it's an addition by malb from 2007!
I don't think it's possible to add a doctest to animate.py, since those require ImageMagick or similar to run.
Sounds reasonable, as I suspected but hoped against.
Sorry about asking the matrix notebook question. I hate it when we discover bugs or unneeded functionality when trying to do very routine fixes. I'd be tempted to just can the function entirely, except there's the deprecation period and we should probably ask Martin if it's needed...
Random annoyance this has nothing to do with.
warning: sage/matrix/../modules/vector_modn_sparse_c.pxi:110:8: Unreachable code warning: sage/matrix/../modules/vector_modn_sparse_c.pxi:136:8: Unreachable code warning: sage/matrix/../modules/vector_modn_sparse_c.pxi:201:8: Unreachable code warning: sage/matrix/../modules/vector_modn_sparse_c.pxi:204:8: Unreachable code warning: sage/matrix/../modules/vector_integer_sparse_c.pxi:264:16: local variable 'tmp' referenced before assignment
Are these actual unused branches or is the compiler just blowing off steam? Anyway, not worrisome here.
For the
sage -n
problem, see #13794. I think the scripts patch at #11409 will fix it.
Yes, I just couldn't remember the ticket numbers, I wasn't actually complaining.
comment:8 Changed 10 years ago by
It looks like visualize_matrix
does one thing that matrix_plot
does not: it lets you put a bound on the size of the figure, and then scales the picture accordingly. So if you have a 20x20 matrix and plot it in a 10x10 picture, each pixel corresponds to the entries in a 2x2 block. Maybe it would make sense to merge this functionality into matrix_plot
and then deprecate visualize_matrix
. Then we wouldn't have to worry about what this should do in the notebook. I agree that it would be more consistent for it to display the picture, but at least the current behavior is documented.
I have no idea about those warnings about unreachable code, by the way.
comment:9 Changed 10 years ago by
Reviewers: | → Karl-Dieter Crisman |
---|---|
Status: | needs_review → positive_review |
Okay, sounds good enough. I'll open a new ticket to ask about this other stuff for visualize_matrix
, then. Thanks for the quick patch!
comment:10 Changed 10 years ago by
Description: | modified (diff) |
---|
That is now #13812.
Patchbot, apply trac_13807_graphics_filename.patch
comment:11 Changed 10 years ago by
Merged in: | → sage-5.5.rc1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
In Sage-5.5.rc0:
Ignoring c files, that means we need to fix
Huh, that's more than I was expecting. Anyway, it's seven files.
I'm making this a 5.5 blocker because of the bug when
EMBEDDED_MODE=True
for showing animate, which is a regression due to a bugfix. I'm not sure whether any of these can actually be doctested, though, and if a few people disagree we can make it critical. I just really hate regressions caused by bug fixes (even when I cause them, luckily not the case here).