Opened 5 years ago

Closed 5 years ago

#16640 closed defect (fixed)

Graphics3d.show abuses graphics_filename

Reported by: gagern Owned by:
Priority: major Milestone: sage-6.4
Component: graphics Keywords:
Cc: Merged in:
Authors: Martin von Gagern, Jeroen Demeyer Reviewers: Jeroen Demeyer, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: 034fa58 (Commits) Commit: 034fa58b0051be5db6942a76c06010e15a3d664b
Dependencies: Stopgaps:

Description

Code in sage/plot/plot3d/base.pyx in the method Graphics3d.show() makes invalid use of graphics_filename: it generates a name (like sage0.png), then strips the extension and appends various other extensions during the course of the method. But graphics_filename makes no guarantees that these other file names will be unused. In fact chances are pretty good that these files already exist from a different run of that method inside the same cell. So we get clashes there. Perhaps if things change in other places we might even get a security issue due to a race condition here.

I can think of two possible approaches: either call graphics_filename in several places, once for every file name we need to generate. Or create a temporary directory and write all files inside that with fixed names. The former will likely be easier to handle for notebook, but may make the code harder to read. Particularly those cases where some tool will use one file name to derive another file name might be a real pain, like when the applet size is encoded in the file name, or the magic aroud the archive_name for jmol. Will need some good interactive testing to make sure that everything still works after these changes.

Change History (64)

comment:1 Changed 5 years ago by gagern

  • Branch set to u/gagern/ticket/16640
  • Created changed from 07/10/14 06:59:14 to 07/10/14 06:59:14
  • Modified changed from 07/10/14 06:59:14 to 07/10/14 06:59:14

comment:2 Changed 5 years ago by gagern

  • Authors set to Martin von Gagern
  • Commit set to 9dfe1337683a6d99118b658fad39d55a0110692a
  • Dependencies set to #16645
  • Status changed from new to needs_review

Note that only the last two commits are new for this ticket; the rest comes from #16645 and its dependencies.

I tested things both in notebook and interactively. java3d failed in both cases, for which I just filed #16647. Apart from that, everything looks fine. In particular it seems as if I got all the file name interdependencies for jmol right.


New commits:

58b9c46minimal fix to restore functionality
16cbd2dMerge branch 'develop' into ticket/16533-stopgap
b3e656bgraphics_filename: return a tmp_filename() if not in EMBEDDED_MODE
69b285dRestore possibly positional arguments.
e3c5542Merge branch ticket/15515 into ticket/16608 to create ticket/16645.
9f30e94Always use graphics_filename instead of tmp_filename.
bb7307fPrevent doctest from leaking file into current working directory.
0dffab6Include dot in extension for graphics_filename().
9dfe133Generate names for 3d graphics files independently.

comment:3 Changed 5 years ago by gagern

These are the changes that need reviewing in this ticket here.

comment:4 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 5 years ago by git

  • Commit changed from 9dfe1337683a6d99118b658fad39d55a0110692a to cf63c2dd372caa602e67397e871ac65c164a2cba

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

301185bInclude dot in extension for graphics_filename().
cf63c2dGenerate names for 3d graphics files independently.

comment:6 Changed 5 years ago by gagern

  • Dependencies #16645 deleted
  • Modified changed from 09/05/14 07:19:46 to 09/05/14 07:19:46

comment:7 Changed 5 years ago by git

  • Commit changed from cf63c2dd372caa602e67397e871ac65c164a2cba to 84a2f30d7d494ece9772605a86b85d3b90742fab

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

61809d6Include dot in extension for graphics_filename().
84a2f30Generate names for 3d graphics files independently.

comment:8 Changed 5 years ago by jdemeyer

  • Branch changed from u/gagern/ticket/16640 to u/jdemeyer/ticket/16640
  • Modified changed from 09/05/14 07:26:24 to 09/05/14 07:26:24

comment:9 Changed 5 years ago by git

  • Commit changed from 84a2f30d7d494ece9772605a86b85d3b90742fab to 49ce20b63e2dc39d611f453f76077620f4faccfb

Branch pushed to git repo; I updated commit sha1. New commits:

1a48c6bUse "with open() as f" syntax
49ce20bSet fg to figsize[0] like before

comment:10 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

Merged with 6.4.beta4 and added some fixes. Please review.

comment:11 Changed 5 years ago by git

  • Commit changed from 49ce20b63e2dc39d611f453f76077620f4faccfb to 463eb63585d45e209938fcd8b8dd91d1a488d2e4

Branch pushed to git repo; I updated commit sha1. New commits:

463eb63Rendering canvas3d in all doctests is too slow

comment:12 Changed 5 years ago by git

  • Commit changed from 463eb63585d45e209938fcd8b8dd91d1a488d2e4 to 711127f7cf89f6e75d7f2e970c4a58ec51ec03c5

Branch pushed to git repo; I updated commit sha1. New commits:

711127fMerge remote-tracking branch 'origin/develop' into ticket/16640

comment:13 follow-up: Changed 5 years ago by kcrisman

Hi Jeroen: Your first two commits are fine, as is the first commit of Martin's. (Currently looking at his main, second one.) How much slower did the doctests become without your last commit? (Martin, if you see this, what was your thinking on changing the doctest mode?)

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by gagern

Hi, sorry I didn't respond to the commits by Jeroen any sooner.

Replying to kcrisman:

Hi Jeroen: Your first two commits are fine,

I agree. Thanks a lot!

(Martin, if you see this, what was your thinking on changing the doctest mode?)

My main reason for modifying that line at all was the fact that canvas3d only works in embedded mode, so having it available otherwise might confuse users. My reson for including it in doctests is simply because all the other possible viewers are active in doctest mode as well. There is a doctest specifically for the canvas3d case, but if I restrict canvas3d to embedded mode only, that one would fail. So staying in line with all the other tests seemed the reasonable thing to do.

If things really are too slow, I'd change that to if viewer == 'canvas3d' and (DOCTEST_MODE or EMBEDDED_MODE): so we can have that one explicit doctest nevertheless. But first I'd like to quantify that effect on test execution time, and right now I have a build problem.

comment:15 Changed 5 years ago by jdemeyer

Always doing canvas3d is really too slow, I remember one file(!) taking over 1000s to doctest.

comment:16 Changed 5 years ago by git

  • Commit changed from 711127f7cf89f6e75d7f2e970c4a58ec51ec03c5 to bd1479d38fe1adb42755ab92499ff8c1ba35592d

Branch pushed to git repo; I updated commit sha1. New commits:

bd1479dMake viewer="canvas3d" an error outside the Notebook

comment:17 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by jdemeyer

Replying to gagern:

My main reason for modifying that line at all was the fact that canvas3d only works in embedded mode, so having it available otherwise might confuse users.

Fixed by raising a suitable exception.

comment:18 in reply to: ↑ 17 Changed 5 years ago by kcrisman

My main reason for modifying that line at all was the fact that canvas3d only works in embedded mode, so having it available otherwise might confuse users.

Fixed by raising a suitable exception.

That was something we needed a while ago anyway! Thanks.

comment:19 follow-ups: Changed 5 years ago by kcrisman

Here is something minor but still needing attention. The documentation says

        -  ``filename`` -- string (default: a temp file); file
           to save the image to

but of course this is now only going to be true in the command line. Which probably makes sense. But this should be noted. Similarly,

        # Tachyon resolution options

should probably just add a quick note that "mode defaults" also are being set in the next few lines.

Interestingly,

            # Temporary hack: encode the desired applet size in the end of the filename:
            # (This will be removed once we have dynamic resizing of applets in the browser.)

and the following code could in principle be removed, as the update does indeed have dynamic resizing! But I think let's leave that be for somewhere else; I don't want to mess with the already messy naming of jmol stuff...

Finally, I don't understand this comment - I'm sorry if I am just thinking inside the box:

                # when testing this, modify graphics_filename so that it
                # returns files which don't automatically share a base name,
                # e.g.: "import random; i = random.randint(0, 10000)"

Was this just a "note to self" or do you mean that, as a tester, I should actually change graphics_filename instead of letting it just keep adding one to base names, or ... ? I figured the whole point of this change was to test where they did share a base name, but again I may be thinking too conventionally.

comment:20 Changed 5 years ago by kcrisman

So far it seems fine, though I rediscovered and diagnosed #11275 along the way.

comment:21 follow-ups: Changed 5 years ago by kcrisman

  • Status changed from needs_review to needs_work

The following in the command line don't seem to create any files (I mean visible ones in my directory). Maybe they aren't supposed to?

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo')
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo',viewer='tachyon')

Note that the second thing pops up a jmol viewer. I can't figure out if this ticket introduced it, though it seems to predate that, based on my experimentation.

Also, with the current documentation, there is no example of how to properly use the filename keyword here, and that should be added.

Anyway, "needs work" to clarify/fix these filename use issues.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 5 years ago by jdemeyer

Replying to kcrisman:

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo')
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo',viewer='tachyon')

The correct syntax is

sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1)).show(filename='foo')

comment:23 in reply to: ↑ 19 ; follow-up: Changed 5 years ago by jdemeyer

Replying to kcrisman:

Here is something minor but still needing attention. The documentation says

        -  ``filename`` -- string (default: a temp file); file
           to save the image to

but of course this is now only going to be true in the command line. Which probably makes sense.

No, it doesn't make sense. An explicit filename should override the default, even in the notebook.

comment:24 in reply to: ↑ 22 ; follow-up: Changed 5 years ago by kcrisman

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo')
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo',viewer='tachyon')

The correct syntax is

sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1)).show(filename='foo')

Good to know. But that sort of proves my point, then, that the documentation (old and new) doesn't tell me that - in this case, because so many keywords can be passed to the plot and then show up in show(). Such as what worked in Sage 6.2 and possibly broken by the new display hook

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),viewer='jmol')  # jmol applet pops up
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),viewer='tachyon') # tachyon-generated png pops up in a viewer

If you confirm that this does work properly to generate the filenames in the command line then I'm okay with making sure there are explicit examples that say it doesn't work from the plot command - preferably in the plot command docs!

comment:25 in reply to: ↑ 23 Changed 5 years ago by kcrisman

Here is something minor but still needing attention. The documentation says

        -  ``filename`` -- string (default: a temp file); file
           to save the image to

but of course this is now only going to be true in the command line. Which probably makes sense.

No, it doesn't make sense. An explicit filename should override the default, even in the notebook.

Hmm, I guess. But I don't know that there is any easy way to find files in a cell directory in the notebook - though I guess a good use case is to save it in the DATA directory, where it of course shouldn't show up in the cell output.

Last edited 5 years ago by kcrisman (previous) (diff)

comment:26 follow-up: Changed 5 years ago by jdemeyer

Any files created in the current directory are picked up by the notebook, so it would actually work!

comment:27 in reply to: ↑ 26 Changed 5 years ago by kcrisman

Any files created in the current directory are picked up by the notebook, so it would actually work!

Sorry, I wasn't clear. I meant that the current code in this ticket doesn't allow the filenames, and if one enabled it, it would be kind of useless since there is no access to the cell directory. My counterexample to that is that one might want to use filename=DATA+'foo' or something to put a graphic file in the data directory, though I haven't tested whether this actually works.

comment:28 in reply to: ↑ 24 Changed 5 years ago by jdemeyer

(never mind)

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:29 in reply to: ↑ 19 Changed 5 years ago by gagern

Replying to kcrisman:

Finally, I don't understand this comment - I'm sorry if I am just thinking inside the box:

                # when testing this, modify graphics_filename so that it
                # returns files which don't automatically share a base name,
                # e.g.: "import random; i = random.randint(0, 10000)"

Was this just a "note to self" or do you mean that, as a tester, I should actually change graphics_filename instead of letting it just keep adding one to base names, or ... ?

Doctests mostly check whether a given file can be created, but to check whether that file displays as intended, you'd use an interactive setup not a doctest. So I envision testers using the notebook to run tests, with no way to programmatically tell whether he is doing tests or something else.

And the notebook numbers files by incrementing some counter until they are unique. I.e. starting by sage0.png then sage1.png. But the critical point here is that the collision detection takes file extensions into account. So if I were to request e.g. a .foo file and a .bar file, I'd get sage0.foo and sage0.bar not sage1.bar. Usually. Unless sage0.bar already exists at the time the graphics file name is requested, in which case I'd get sage0.foo but sage1.bar.

The problematic case I had in mind was one where some viewer uses one file name to derive some other file name. So if a viewer were to open sage0.foo and from that name infer that it also has to open sage0.bar, then this would work during test cases, but might fail unexpectedly if a user creates sage0.bar in some other code inside the same cell.

I'd like to detect such situations. I'd like to make sure that if there is more than one file required for some viewer, then all file names are stated explicitly somewhere, and not inferred by some extension replacement. And that's the point of the comment. I'd like to break the sequential numbering and replace it by random numbers to ensure that inferred file names would break.

So yes, anyone who tests a patch that might be affected by the scenario outlined above should, in my opinion, temporarily modify the code of graphics_filename to ensure that no file name inference is used. If you can think of a better wording to convey this idea, please let me know. I guess we could also add a link to this comment here, since the whole explanation is too long to put into such a comment, imho.

comment:30 Changed 5 years ago by jdemeyer

In vanilla (i.e. without this patch) sage-6.4.rc0 in the notebook, when doing

cube(viewer="jmol")

I get

/usr/local/src/sage-config/local/lib/python2.7/site-packages/IPython/cor\
e/formatters.py:239: FormatterWarning: Exception in text/plain
formatter: Jmol failed to create file
'/tmp/tmpE8eQHg/.jmol_images/sage0-size500.jmol.png', see
'/home/jdemeyer/.sage/temp/tamiyo/24593/tmp_rBkFiX.txt' for details
  FormatterWarning,
None

(not the clearest error message) followed by a blank red square.

Looking at the file mentioned for details gives

Exception in thread "main" java.lang.UnsupportedClassVersionError: org/jmol/translation/Jmol/en_GB/Messages_en_GB : Unsupported major.minor version 51.0
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(ClassLoader.java:634)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:277)
        at java.net.URLClassLoader.access$000(URLClassLoader.java:73)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:212)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:205)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:321)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:294)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:266)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:188)
        at org.jmol.api.Interface.getInterface(Unknown Source)
        at org.jmol.i18n.Resource.getResource(Unknown Source)
        at org.jmol.i18n.GT.addBundle(Unknown Source)
        at org.jmol.i18n.GT.addBundles(Unknown Source)
        at org.jmol.i18n.GT.<init>(Unknown Source)
        at org.jmol.i18n.GT.getTextWrapper(Unknown Source)
        at org.jmol.i18n.GT._(Unknown Source)
        at org.openscience.jmol.app.JmolApp.getOptions(Unknown Source)
        at org.openscience.jmol.app.JmolApp.parseCommandLine(Unknown Source)
        at org.openscience.jmol.app.JmolData.main(Unknown Source)

Does this mean my Java version is too old?

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:31 Changed 5 years ago by git

  • Commit changed from bd1479d38fe1adb42755ab92499ff8c1ba35592d to 3852a0a23966400e02a14512e97e8ae46e45ec2a

Branch pushed to git repo; I updated commit sha1. New commits:

3852a0aFurther improve 3d graphics file handling

comment:32 Changed 5 years ago by jdemeyer

I think this patch should fix the issues mentioned above (apart from Martin's comment which I decided to simply remove, note that doctests also generate random filenames).

comment:33 follow-up: Changed 5 years ago by jdemeyer

The error in 30 occurs with or without this patch, but I would still like to know what is wrong.

comment:34 Changed 5 years ago by jdemeyer

With this patch, the filename= option does work in the notebook.

comment:35 Changed 5 years ago by jdemeyer

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Karl-Dieter Crisman
  • Status changed from needs_work to needs_review

comment:36 in reply to: ↑ 33 ; follow-up: Changed 5 years ago by kcrisman

The error in 30 occurs with or without this patch, but I would still like to know what is wrong.

That indicates to me that your notebook instantiation is somehow corrupted and/or the display hook stuff didn't even come through. Earlier today I had it all working, for example, and now it doesn't even have the extra click boxes in the template, mysteriously. I'm baffled. But that shouldn't happen. Though you may be right about the old java.

Last edited 5 years ago by kcrisman (previous) (diff)

comment:37 in reply to: ↑ 36 ; follow-up: Changed 5 years ago by kcrisman

cube(viewer="jmol")

The error in 30 occurs with or without this patch, but I would still like to know what is wrong.

That indicates to me that your notebook instantiation is somehow corrupted and/or the display hook stuff didn't even come through. Earlier today I had it all working, for example, and now it doesn't even have the extra click boxes in the template, mysteriously. I'm baffled. But that shouldn't happen. Though you may be right about the old java.

Yeah, I can't replicate this here, though the old java could possible contribute. What happens if you click on the "live 3d" box and then evaluate? That would bypass java creating the static image.

comment:38 in reply to: ↑ 37 Changed 5 years ago by jdemeyer

Replying to kcrisman:

What happens if you click on the "live 3d" box and then evaluate?

When enabling "Load 3-D Live" I get the horrifying error message (it's Halloween, right?)

/usr/local/src/sage-git/local/lib/python2.7/site-packages/IPython/core/f\
ormatters.py:239: FormatterWarning: Exception in text/plain formatter:
Jmol failed to create file
'/tmp/tmpJEIc27/.jmol_images/sage0-size500.jmol.png', see
'/home/jdemeyer/.sage/temp/tamiyo/2874/tmp_IABUbj.txt' for details
  FormatterWarning,
None

then some 3d figure which does work nicely, then the sentence "Right-click to get options menu."

comment:39 Changed 5 years ago by jdemeyer

When enabling "Use java for 3-D" and not "Load 3-D Live" I get a very similar result (with one interesting difference: the right-click menu uses a different font and is in English, with "Load 3-D Live" the menu was in Dutch).

(edit: cannot reproduce this anymore, getting a light grey square like in 40) (edit2: restarting the browser fixed this again)

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:40 Changed 5 years ago by jdemeyer

When enabling both "Use java for 3-D" and "Load 3-D Live" I get a result similar to 30 except that the blank square is light grey instead of red.

comment:41 Changed 5 years ago by jdemeyer

For the record, I am using

java version "1.6.0_31"
OpenJDK Runtime Environment (IcedTea6 1.13.3) (Gentoo build 1.6.0_31-b31)
OpenJDK 64-Bit Server VM (build 23.25-b01, mixed mode)

and

Firefox ESR 24.8.0

comment:42 Changed 5 years ago by kcrisman

I won't have time to look at this over the weekend, nor do I have the jmol/jsmol expertise to diagnose it, but I would recommend putting something about it on the 6.4.rc0 release thread so that Jonathan Gutow sees it.

comment:43 follow-up: Changed 5 years ago by strogdon

I don't know if anyone has tried it but I removed the patches associated with #17170 from 6.4.r0 and I still have the issue with viewer='tachyon' opening a jmol window instead of creating a png. So perhaps someother displayhook-related ticket?

comment:44 in reply to: ↑ 43 ; follow-up: Changed 5 years ago by kcrisman

I don't know if anyone has tried it but I removed the patches associated with #17170 from 6.4.r0 and I still have the issue with viewer='tachyon' opening a jmol window instead of creating a png. So perhaps someother displayhook-related ticket?

Hmm. Do you know where you first saw this? (I'm not suggesting you do a full bisection! Just wondering.)

comment:45 follow-up: Changed 5 years ago by kcrisman

I've tried to report the things on this ticket not actually related to this ticket to sage-release. However, now I've forgotten where we were in the review!

comment:46 in reply to: ↑ 45 Changed 5 years ago by gagern

Replying to kcrisman:

I've forgotten where we were in the review!

The commit from comment:31 seems to deal with all outstanding issues from comment:19, so if you review that, I believe we should be fine. I guess Jeroen contributed enough that he should be an author as well. Do you want to file a ticket for the dynamic resizing of applets you mentioned in comment:19?

comment:47 in reply to: ↑ 44 Changed 5 years ago by strogdon

Replying to kcrisman:

I don't know if anyone has tried it but I removed the patches associated with #17170 from 6.4.r0 and I still have the issue with viewer='tachyon' opening a jmol window instead of creating a png. So perhaps someother displayhook-related ticket?

Hmm. Do you know where you first saw this? (I'm not suggesting you do a full bisection! Just wondering.)

I believe on sage-release. But this is not related to this ticket. I only saw your comment:21 above. It should be addressed somewhere.

comment:48 in reply to: ↑ 21 Changed 5 years ago by jdemeyer

Replying to kcrisman:

sage: var('y')
y
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo')
sage: plot3d(x^2+y^3,(x,-1,1),(y,-1,1),filename='foo',viewer='tachyon')

Note that the second thing pops up a jmol viewer.

This is now #17284.

comment:49 Changed 5 years ago by jdemeyer

  • Authors changed from Martin von Gagern to Martin von Gagern, Jeroen Demeyer

For the record: I give positive_review to everything on this ticket not authored by me.

comment:50 follow-up: Changed 5 years ago by kcrisman

  • Status changed from needs_review to needs_work

And I am happy with everything except I need to look at the very last patch one more time... okay, looks good other than two questions.

Question - is this a Python syntax I'm not aware of?

def show(self, *, filename=None, **kwds):

In particular, the extra * mystifies me. (Edit - okay, I found it, but it appears to be Python 3 only? I believe it works in recent Python 2.x since you used it, but I couldn't find a definitive reference - even the Python reference was ambiguous.)

Oh, and maybe add a doctest for this commit with the error raised? Maybe that's too trivial to do... but for some reason the doctest

sage: p.show(viewer='canvas3d')

doesn't raise an error, which makes sense because of the code. But it also is now doctested with the most recent change you made, despite what you just said earlier that making canvas3d guys was too long!

Somehow this should be resolved. I think the best way is to doctest the creation of the file, perhaps by setting a one-time flag or something, rather than the error - as long as we tell people that in the command line it will give an error. (Well, we could doctest both as well!)

comment:51 in reply to: ↑ 50 ; follow-up: Changed 5 years ago by gagern

Replying to kcrisman:

In particular, the extra * mystifies me. (Edit - okay, I found it, but it appears to be Python 3 only? I believe it works in recent Python 2.x since you used it, but I couldn't find a definitive reference - even the Python reference was ambiguous.)

This syntax for keyword-only parameters comes from Python 3 but is supported by Cython even for older versions of Python. In #16607 I suggested support for keyword-only parameters in pure Python code, and there jdemeyer pointed out that Cython support.

comment:52 in reply to: ↑ 51 ; follow-up: Changed 5 years ago by kcrisman

  • Status changed from needs_work to needs_info

In particular, the extra * mystifies me. (Edit - okay, I found it, but it appears to be Python 3 only? I believe it works in recent Python 2.x since you used it, but I couldn't find a definitive reference - even the Python reference was ambiguous.)

This syntax for keyword-only parameters comes from Python 3 but is supported by Cython even for older versions of Python. In #16607 I suggested support for keyword-only parameters in pure Python code, and there jdemeyer pointed out that Cython support.

Aha.

With this patch, the filename= option does work in the notebook.

And it really does save to where needed, awesome! Especially in DATA could be useful.

despite what you just said earlier that making canvas3d guys was too long!

Okay, sorry, I see what you did now - got a little too excited there. What do you think about testing the warning?

Otherwise this was good teamwork by you two, very nice.

comment:53 in reply to: ↑ 52 Changed 5 years ago by jdemeyer

Replying to kcrisman:

despite what you just said earlier that making canvas3d guys was too long!

Okay, sorry, I see what you did now - got a little too excited there. What do you think about testing the warning?

I'm too confused, what are you (plural) trying to say here?

comment:54 follow-up: Changed 5 years ago by kcrisman

Here is the short version:

p.show(viewer='canvas3d')

raises an error in the command line but not during doctesting because of the way you wrote the code (creates the file). Perhaps the warning should also be doctested.

comment:55 in reply to: ↑ 54 ; follow-up: Changed 5 years ago by jdemeyer

Replying to kcrisman:

Perhaps the warning should also be doctested.

That's possible but requires some messing around with DOCTEST_MODE. Given that it's a minor thing and that Volker has plans to change all this code anyway, I feel that it's not worth the trouble. But if you insist, I will add that test.

PS: I assume you mean "error", not "warning".

comment:56 in reply to: ↑ 55 Changed 5 years ago by kcrisman

  • Status changed from needs_info to positive_review

Perhaps the warning should also be doctested.

That's possible but requires some messing around with DOCTEST_MODE.

Yes.

Given that it's a minor thing and that Volker has plans to change all this code anyway, I feel that it's not worth the trouble. But if you insist, I will add that test.

Ok.

PS: I assume you mean "error", not "warning".

Yes, though I recall writing both of those a few times and then accidentally closing the browser/getting timed out!

comment:57 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Conflicts with 6.4, please merge.

comment:58 Changed 5 years ago by git

  • Commit changed from 3852a0a23966400e02a14512e97e8ae46e45ec2a to e83f839da144e1d90e495b953ef3f5861d341f41

Branch pushed to git repo; I updated commit sha1. New commits:

e83f839Merge tag '6.4' into ticket/16640

comment:59 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to positive_review

Done, the conflict was the removal of a redundant import of tmp_filename.

comment:60 Changed 5 years ago by jdemeyer

  • Status changed from positive_review to needs_work
sage -t --long src/sage/repl/display/formatter.py
**********************************************************************
File "src/sage/repl/display/formatter.py", line 320, in sage.repl.display.formatter.SageNBTextFormatter.__call__
Failed example:
    fmt(FooGraphics())
Expected:
    showing graphics
    ''
Got:
    showing graphics
    doctest:239: FormatterWarning: Exception in text/plain formatter: [Errno 2] No such file or directory: '/nonexistent.png'
**********************************************************************

comment:61 Changed 5 years ago by git

  • Commit changed from e83f839da144e1d90e495b953ef3f5861d341f41 to 034fa58b0051be5db6942a76c06010e15a3d664b

Branch pushed to git repo; I updated commit sha1. New commits:

034fa58Fix doctest to use existing file

comment:62 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:63 Changed 5 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:64 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/16640 to 034fa58b0051be5db6942a76c06010e15a3d664b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.