Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change plotting #7

Closed
wants to merge 1 commit into from
Closed

change plotting #7

wants to merge 1 commit into from

Conversation

woutersj
Copy link

An attempt to resolve issue #6. tmplot already checks for an empty plot, so do we need TM_OCTAVE_PLOT_DIGEST?

Note that gcf() has the side effect of creating an empty plot if none is open, get (groot, "currentfigure"); doesn't do this
Found this in the Octave docs.

@da-liii da-liii self-requested a review February 6, 2021 00:12
@da-liii
Copy link
Contributor

da-liii commented Feb 6, 2021

TM_OCTAVE_PLOT_DIGEST is introduced by me. I didn't write a test case for this case. Let me add some test cases for the octave plugin to clarify the functionality.

@da-liii
Copy link
Contributor

da-liii commented Feb 6, 2021

I tried to figure out why I introduced the global digest in octave. But found that it does not work correctly. This PR looks good. And let us add some test cases for it in test/plots.tm.

@@ -36,17 +36,9 @@ function tmrepl()
eval (__r, "tmlasterr");

if disp_ans
global TM_OCTAVE_PLOT_DIGEST;

updated_digest= hash ("md5", serialize (get(gcf())));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only usage for octave/kernel/serialize.m. If we remove serialize, octave/kernel/serialize.m can also be removed.

@woutersj
Copy link
Author

woutersj commented Feb 7, 2021

It seems that the global digest does not work well because it is hashing the current figure, which is not necessarily altered by plotting. A figure has an axis child, which in turn has objects as its children. The serialization of figure object is therefore not a one-to-one mapping with what is shown on screen.

I have done some more experiments to see how the Octave REPL handles figures. Octave handles graphics and the ans returned by the command as separate things. For example

clf;
A = rand (100);
[X, Y] = find (A > 0.95);
imshow (A);

pops up a plot window, even though there is no output on the console, because of the semicolons. In TeXmacs this code would not result in a plot.

To complicate matters a user can also open several figure in one cell:

figure(1)
fplot (@sin, [-10, 10])
figure(2)
fplot (@cos, [-10, 10])

If we plot when output is requested (no ; at the end) and clear the plot (as in this PR), then this would not behave naturally if someone enters non-plotting commands after the plotting commands, e.g.

clf;
A = rand (100);
[X, Y] = find (A > 0.95);
imshow (A);
det(A)

If on the other hand we use an improved hashing function to detect changes and only plot when a figure has been altered, this may be inconvenient if a user wants to build up a complex plot in several separate cells (layering multiple graphs, adding a legend, manipulating axes/titles etc.). The plot would show up many times, after every evaluated line. This could of course be circumvented with multiline input.

It seems to me that showing the plot in every cell that has altered a figure, independent of the presence of a semicolon, would be the closest to the Octave REPL behaviour. This would require an improvement of the figure hashing first, though.

@da-liii
Copy link
Contributor

da-liii commented Feb 8, 2021

Thanks for your detailed explanation. All of these explanation can be kept in test/plots.tm. test/plots.tm is blank now. I wonder if I forgot to upload the test cases.

Looking into the corresponding issue of this PR, we'd better to fix it rather than to skip it by introducing other flaws.

@da-liii da-liii closed this Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants