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

Refactor so that analysis functions can more easily be called #78

Closed

Conversation

ejeschke
Copy link
Contributor

This expands support for calling the imexamine functions and collecting the output from an external entity. The following summarizes the main changes:

  • remove matplotlib pylab ("plt") calls from anywhere that an external figure is passed in, because these are not compatible.
  • add a "out_f" kwarg that can be passed in to methods that use print() to return results. The out_f allows the output to be collected in a string buffer so that it can be displayed somewhere other than a terminal.

This expands support for calling the imexamine functions and collecting
the output from an external entity.  The following summarizes the main
changes:

- remove matplotlib pylab ("plt") calls from anywhere that an external
  figure is passed in, because these are not compatible.

- add a "out_f" kwarg that can be passed in to methods that use print()
  to return results.  The out_f allows the output to be collected in
  a string buffer so that it can be displayed somewhere other than a
  terminal.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 27.447% when pulling f06e20b on ejeschke:imexam-extcall-enhance into 945c235 on spacetelescope:master.

@pllim
Copy link
Contributor

pllim commented Nov 18, 2016

Test failure is due to Sphinx warning; seems unrelated at first glance. IMHO, the new keyword should also be documented in docstrings of affected functions.

@ejeschke
Copy link
Contributor Author

I had originally added it to all the docstrings, but then I noticed that the fig parameter was undocumented, so I wasn't sure about adding it. Probably we should add fig and out_f.

@pllim
Copy link
Contributor

pllim commented Nov 18, 2016

Ah... Well, we'll see what @sosey says.

@ejeschke
Copy link
Contributor Author

Did you try the plugin, @pllim ? What did you think?

@pllim
Copy link
Contributor

pllim commented Nov 18, 2016

@ejeschke , the plugin is cool! However, took me a minute to remember that the generated plots are in a different window that looks like a different channel but is not really a channel. Being used to other plugins like Histogram, intuitively I was expecting the plot to appear within the plugin itself (on the right panel). But I see that this implementation is more flexible in allowing multiple plots to stay on display.

However, I cannot figure out how to:

  1. Close the plot. Clicking "x" does nothing.
  2. Save the plot. I can technically take a screenshot but that is not ideal.

@pllim
Copy link
Contributor

pllim commented Nov 18, 2016

Update: I could save the latest plot with "s" but seems like the name is hardcoded (risk of overwriting previous save). And that still does not allow me to save an older plot on a separate window.

@ejeschke
Copy link
Contributor Author

Close the plot. Clicking "x" does nothing.

Yes, TBD. This is noted in the CAVEATS in the docstring. However, as a workaround you can make sure the plot has the focus and then use the "-" button in the workspace toolbar to close it. It's a good idea to "detach" it first.

I could save the latest plot with "s" but seems like the name is hardcoded (risk of overwriting previous save). And that still does not allow me to save an older plot on a separate window.

The hardcoding seems to be the way imexam does it. That could be changed with the appropriate GUI elements added to the plugin, of course.

@ejeschke
Copy link
Contributor Author

Being used to other plugins like Histogram, intuitively I was expecting the plot to appear within the plugin itself (on the right panel).

This is also a possibility. If we wanted to do that we could just make the plots in a tabbed notebook in the plugin GUI. When you detach a plot it just makes a new tab in the notebook where the new plots start showing up. That would certainly be more useful with the other styles of workspace configuration, but not as easy to resize the plots.

@sosey
Copy link
Member

sosey commented Nov 28, 2016

Some additional notes....(i've had this window open since last week, sorry for the delay)

I could save the latest plot with "s" but seems like the name is hardcoded (risk of overwriting previous save). And that still does not allow me to save an older plot on a separate window.

>The hardcoding seems to be the way imexam does it. That could be changed with the appropriate GUI elements added to the plugin, of course.

There's a default name which is hardcoded, but the user can change the name of the plot anytime.

a=imexam.connect()

In [5]: a.plotname()
imexam_plot.pdf

In [6]: a.plotname="test.pdf"

In [7]: a.plotname
Out[7]: 'test.pdf'

or use the access function set_plot_name(filename), which I see I only have attached at the exam level, not the connect level( changed in the logger update). In the example above that would be a.exam.set_plot_name(filename). The initial figure name starts as "imexam" which gets appended with the number of the plot window, since you can have multiple plot windows open at the same time. My original thought had been people are using the exam loop for quick analysis and not saving plots in that mode often. They could alter the name before or after the call, or change the filename from another terminal during, or later go back and call without the viewer and return the data and make a more specific plot for themselves. I could change this to randomize the filename so that multiple different plots from the same plotting window would save to separate files. This might accommodate the plugin better.

Since under the DS9 interaction I have to block the terminal, changing base names can be done when you are not in imexam() mode. For the ginga html5 window you can set the name in the terminal when you want. If the plugin provides an input frame to enter a plot name that could possibly be sent to the filename function on-press or something.

The full documentation is something I've fallen behind on, I haven't had much time this year to work on imexam, so feel free to update the docs as needed, assume omissions are just things I haven't gotten around to yet :(

add a "out_f" kwarg that can be passed in to methods that use print() to return results. The out_f allows the output to be collected in a string buffer so that it can be displayed somewhere other than a terminal.

Is there a way we can do this with the logging functionality and catching with the StreamHandler I already have using sys.stdout? I had a couple issues open to fix the logging, I combined these last week and made the prints logger calls to the stdout stream to accommodate 1) fixing the logging issues I already had 2) letting users turn off screen messages all together while still logging to file, or not. I played with the plugin a bit, removing the fig, removing the StreamHandler I now have from the imexam logger, and then redirecting to StringIO appears to do the trick and I can get messages to the widget and the plots pop up outside the ginga gui (is there a way to make them not do that?)

remove matplotlib pylab ("plt") calls from anywhere that an external figure is passed in, because these are not compatible.

Ah, the fig stuff. Originally, I had added the fig parameter to facilitate multiple figures and switching between them. In the end, I decided it was better to have one figure, then freeze it when the user wanted to keep it on the screen and open a new figure for future plotting. I think we can just remove the fig parameter from the function signatures and be fine, I don't think this functionality is being used anywhere else. The code keeps track of the current figure.

I pushed the updated imexam with fixed logging to my repo under the fix_logging branch. I stuck an edited copy of the Imexam plugin updates here: https://github.com/sosey/playground/tree/ginga

@ejeschke Take a look and see what you think? If it works for you, I can commit the imexam changes which require nothing special for ginga, and then a small update to the plugin like above should make things happy? Lemme know what you think about adding a random filename to the save plot function as well. It'll be really cool if this all works 😄

@ejeschke
Copy link
Contributor Author

Ah, the fig stuff. Originally, I had added the fig parameter to facilitate multiple figures and switching between them. In the end, I decided it was better to have one figure, then freeze it when the user wanted to keep it on the screen and open a new figure for future plotting. I think we can just remove the fig parameter from the function signatures and be fine, I don't think this functionality is being used anywhere else. The code keeps track of the current figure.

@sosey, the passed fig parameter is critical to being able to get the plots to plot in a figure inside the GUI somewhere. When the non-OO ("plt") interface is used, matplotlib wants to manage its own windows and you cannot control where they show up, or possibly even what toolkit is used.

@ejeschke
Copy link
Contributor Author

I think as long as you leave the fig parameter in, and preserve the changes in the PR changing plt calls to regular matplotlib OO calls, the redirected stream can be probably be used to show the output in the text widget, while allowing the plots to show up in the viewer.

@sosey
Copy link
Member

sosey commented Nov 29, 2016

@ejeschke yup, the oo fixes are in there, those should have been cleaned up long ago, thanks!

Ah, I didn't think the plugin was using the fig param in the calls to the imexam plots, lemme go look closer, it's certainly something I can add back into the imexamine class.

sosey added a commit to sosey/imexam that referenced this pull request Dec 28, 2016
sosey added a commit to sosey/imexam that referenced this pull request Dec 31, 2016
sosey added a commit that referenced this pull request Dec 31, 2016
@sosey
Copy link
Member

sosey commented Dec 31, 2016

@ejeschke made some minor changes to the plugin to make it work with the updated imexam logging and added these to the pull request I just merged at 0a6f014

sending you a pull with the update file..

@pllim
Copy link
Contributor

pllim commented Jan 1, 2017

I think this can be closed?

@pllim pllim closed this Jan 1, 2017
@sosey
Copy link
Member

sosey commented Jan 1, 2017

I had intended to leave this open until erik had a chance to try it out, but I guess he can open a new issue and just reference this if there are things to be changed.

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.

4 participants