kylebarron / stata_kernel

A Jupyter kernel for Stata. Works with Windows, macOS, and Linux.
https://kylebarron.dev/stata_kernel/
GNU General Public License v3.0
264 stars 57 forks source link

graph display bug #391

Closed ZHULOO closed 2 years ago

ZHULOO commented 3 years ago

Problem description

Debugging log

If possible, attach the text file located at

$HOME/.stata_kernel_cache/console_debug.log

where $HOME is your home directory. This will help us debug your problem quicker.

NOTE: This file includes a history of your session. If you work with restricted data, do not include this file.

Code Sample

Especially if you cannot attach the debugging log, please include a minimal, complete, and verifiable example.

use "https://stats.idre.ucla.edu/stat/stata/notes/hsb2", clear
scatter read math, title("Reading score vs Math score")
scatter math science, title("Math score vs Science score")

Expected Output

image

unexpected output

Just display like this,

image

If you didn't attach the debugging log, please provide:

mcaceresb commented 3 years ago

@ZHULOO Does it display if you add graph display after the commands are run?

simon-d-s commented 3 years ago

@mcaceresb : no, it does not display. I think it is because Stata 17 has changed the prompt after graph export. Before, it said (file path/foo.png written in PNG format). Now, it says file path/foo.png saved as PNG format.

mcaceresb commented 3 years ago

@simon-d-s Ah, interesting. That sounds straightforward if that's all it is. Try installing from this branch. If it works then I'll make a PR.

simon-d-s commented 3 years ago

@mcaceresb unfortunatly, it's not all. it seems that under Stata 17, capture noisily suppresses the (note: file foo.png not found) issued in earlier versions of Stata. This seems to be relevant for the first line of clean_log_eol() in stata_session.py, if my reading of the code is correct (sorry, I'm a python beginner)

mcaceresb commented 3 years ago

@simon-d-s Ah, well, then I don't think I will fix until I can test this on Stata 17. Might be a while.

kylebarron commented 3 years ago

If anyone is on Stata 17 and would like to take a deeper look and submit a Pull Request, that might move things a bit faster.

ZHULOO commented 3 years ago

Thanks you guys for the discussion, when I run scatter math science, title("Math score vs Science score"), The Stata17 has generated the " .png" figure in the cached directories,But It just does not display the figure in the notebook.

simon-d-s commented 3 years ago

First: apologies, my above statement:

@mcaceresb unfortunatly, it's not all. it seems that under Stata 17, capture noisily suppresses the (note: file foo.png not found) issued in earlier versions of Stata. This seems to be relevant for the first line of clean_log_eol() in stata_session.py, if my reading of the code is correct (sorry, I'm a python beginner)

... is wrong (it seems that I've created a mess fiddling around simultaneously with Stata 16 and 17...).

What is correct, however, is the following:

Obviously this makes it difficult to define g_exp in expect() for both Stata 16 and 17. -> it may be necessary to add a config-setting for the Stata-version in use (or read it from the c(version) in Stata?).

For my own use, I adapted g_expect to g_exp = r'(?<![(])file {}'.format(self.cache_dir_str[:34]), changed the regex in clean_log_eol() to r'^\((?:note: )?file {}/graph\d+\.({}) not found\)'... and left the one in expect_graph() unchanged compared to #392, which fixes the issue.

mcaceresb commented 3 years ago

@simon-d-s Can you try it from this branch? I've made the note: optional in the regex. It's easier than trying to change it based on the user's state version, and given the not found is still there I think it's probably good enough.

simon-d-s commented 3 years ago

@mcaceresb , sorry for not being clear enough. The main issues stems from g_exp on line 302 of stata_session.py

The existing code g_exp = r'\(file {}'.format(self.cache_dir_str[:34]) will capture the beginning of (file path/foo.png not found) issued by Stata 17, instead of the beginning of file path/foo.png saved as PNG format. => works for Stata 16, but not for Stata 17.

Conversely, if I use g_exp = r'(?<!\()file {}'.format(self.cache_dir_str[:34]) in order to make it work with Stata 17, it will not work with Stata 16.

So, the problem is that r'\(file {}'.format(self.cache_dir_str[:34])' exists in both Stata 16 and Stata 17, but they appear in different contexts.

mcaceresb commented 3 years ago

@simon-d-s Does it work if you set: g_exp = r'\(?file {}'.format(self.cache_dir_str[:34])?

ZHULOO commented 3 years ago

The notebook terminal display like this notebook

simon-d-s commented 3 years ago

@mcaceresb It will work, but it will capture both the (file path/foo.png not found) from Stata 17 and the (file path/foo.png written in PNG format) from Stata <17.

I've created a PR with a possible solution for disentangling the two. Not sure whether it's a very elegant way of doing it, but it seems to work.

@mcaceresb could you cross check with Stata 16?

binyamink commented 3 years ago

Hi, I'm experiencing a similar issue. I work from Atom on Windows 10, Stata 14, kernel version 1.12.2. The graph is created in the cache folder but not displayed and the session gets stuck. I attach the debugging log. I tried to use the simple example from above.

console_debug.log

Thanks a lot!

izumis007 commented 3 years ago

I am having the exact same problem. I used older versions of stata_kernel and Stata 17 without any problems at all. But, today I updated to the new version of stata_kernel, and the graph suddenly wouldn't be displayed and I got stuck...

gaksaray commented 3 years ago

@binyamink @izumis007 I had the same problem using the kernel with Stata 17 but this seems to be solved in https://github.com/kylebarron/stata_kernel/pull/394 by @simon-d-s (at least for Stata 17). So you can install the kernel as usual and just replace the stata_session.py file with this version and it should work until we get a new version of the kernel. I actually just created a fork that streamlines this process for myself so you can try that too.

izumis007 commented 3 years ago

@gaksaray Thank you for your advice! I'll try it out next week and report back with the results.

izumis007 commented 3 years ago

@gaksaray It fixed! Thank you. pip install didn't work well so I just copied your version of stata_session.py and replaced it with the older one.