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

Bokeh_class #1899

Merged
merged 9 commits into from Oct 3, 2023
Merged

Bokeh_class #1899

merged 9 commits into from Oct 3, 2023

Conversation

hamogu
Copy link
Contributor

@hamogu hamogu commented Sep 18, 2023

This is a fully working implementation, that could be merged as is.
However, there are a few UI improvements that might be worthwile, e.g. there is no color cycline and the bokeh.plot.show could do with a nice wrapper in sherpa.

@hamogu hamogu added general:visualization note:needs SDS testing PR requires SDS testing and feedback labels Sep 18, 2023
@hamogu hamogu mentioned this pull request Sep 18, 2023
3 tasks
@hamogu
Copy link
Contributor Author

hamogu commented Sep 18, 2023

This PR looks a lot more intimidating than it is, because it is build on top of #1382. Only the last commit (at the time of writing) is actually new. Locally, this passes almost all ploting tests we have, but I've not activated them in this PR, because it only passes "almost" all plotting tests - see #1887. When we get close to merging, I csn simply add bokeh as a new requirement on CI and special case those few tests that fail; but that's not a priority here.

It's more important that @DougBurke and @anetasie take a look at the current functionality.

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #1899 (1d675f5) into main (0a257aa) will decrease coverage by 13.69%.
Report is 31 commits behind head on main.
The diff coverage is 14.18%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1899       +/-   ##
===========================================
- Coverage   81.41%   67.72%   -13.69%     
===========================================
  Files          75       78        +3     
  Lines       27227    27861      +634     
  Branches     4130     4010      -120     
===========================================
- Hits        22166    18870     -3296     
- Misses       4871     8871     +4000     
+ Partials      190      120       -70     
Files Coverage Δ
sherpa/conftest.py 81.98% <ø> (-11.40%) ⬇️
sherpa/plot/__init__.py 83.22% <100.00%> (-1.15%) ⬇️
sherpa/plot/backends.py 96.65% <100.00%> (+0.05%) ⬆️
sherpa/plot/utils.py 94.73% <ø> (ø)
sherpa/plot/pylab_backend.py 2.66% <20.00%> (-87.51%) ⬇️
sherpa/plot/testing.py 72.97% <72.97%> (ø)
sherpa/plot/bokeh_backend.py 2.97% <2.97%> (ø)

... and 37 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a257aa...1d675f5. Read the comment docs.

docs/indices.rst Outdated Show resolved Hide resolved
docs/indices.rst Outdated Show resolved Hide resolved
sherpa/conftest.py Outdated Show resolved Hide resolved
sherpa/plot/backends.py Outdated Show resolved Hide resolved
@hamogu
Copy link
Contributor Author

hamogu commented Sep 18, 2023

EDIT This comment was posted here erroneously and it does not apply to this PR.

I added the test and rebased to pick up the uo-to-date RTD configuration file (to fix the RTD failure in the previous run)

@DougBurke
Copy link
Contributor

@hamogu - you say you rebased this - in a comment an hour or so ago at the time of writing - but the GH still lists fafc0cc as the last commit, which was made 4 hours ago. Was your "rebase" comment about the fafc0cc code?

@DougBurke
Copy link
Contributor

I've got this far before I've run out of TUITs today:

In [1]: from sherpa import plot

In [2]: plot.PLOT_BACKENDS
Out[2]: 
{'BaseBackend': sherpa.plot.backends.BaseBackend,
 'BasicBackend': sherpa.plot.backends.BasicBackend,
 'IndepOnlyBackend': sherpa.plot.backends.IndepOnlyBackend,
 'pylab': sherpa.plot.pylab_backend.PylabBackend,
 'PylabErrorArea': sherpa.plot.pylab_area_backend.PylabErrorArea,
 'BokehBackend': sherpa.plot.bokeh_backend.BokehBackend}

In [3]: plot.set_backend("bokeh")
---------------------------------------------------------------------------
IdentifierErr                             Traceback (most recent call last)
Cell In[3], line 1
----> 1 plot.set_backend("bokeh")

File ~/sherpa/sherpa-xspec/sherpa/plot/__init__.py:177, in set_backend(new_backend)
    175         backend = PLOT_BACKENDS[new_backend]()
    176     else:
--> 177         raise IdentifierErr('noplotbackend', new_backend,
    178                             list(PLOT_BACKENDS.keys()))
    179 # It's a class and that class is a subclass of BaseBackend
    180 elif isinstance(new_backend, type) and issubclass(new_backend, BaseBackend):

IdentifierErr: 'bokeh' is not a valid plotting backend, choose from ['BaseBackend', 'BasicBackend', 'IndepOnlyBackend', 'pylab', 'PylabErrorArea', 'BokehBackend']

In [4]: plot.set_backend("BokehBackend")

@DougBurke
Copy link
Contributor

I did just try the example-plots backend and added a call to

plot.set_backend("BokehBackend")

but then the <plot object>.plot() calls didn't seem to do anything. What am I doing wrong [I also took out the %matplotlb inline line and restarted the kernel just in case that messed things up. Is there a bokeh-equivalent to this magic (although I'm never sure when this magic is needed anymore).

@hamogu
Copy link
Contributor Author

hamogu commented Sep 18, 2023

I added the test and rebased to pick up the uo-to-date RTD configuration file (to fix the RTD failure in the previous run)

That comment is in the wrong PR... it was meant for the link-parameters PR. I ahve too many tabs open that tool almost, but not quite identical. Sorry for the confusion.

@hamogu
Copy link
Contributor Author

hamogu commented Sep 18, 2023

@DougBurke Are you running in the notebook or on a traditional command line?

See https://sherpa--1899.org.readthedocs.build/en/1899/plots/bokeh_backend.html#module-sherpa.plot.bokeh_backend for display help.

But yes, it should be plot.set_backend("BokehBackend") (I fixed the locally already, but did not push again), and yes, the user interface of "how do I get this plot" can be improved. Suggestions welcome.

@DougBurke
Copy link
Contributor

DougBurke commented Sep 19, 2023

So, I start with "jupyter lab" [*] and then have

import numpy as np

from sherpa import data
from sherpa.astro import data as astrodata

from sherpa import plot
from sherpa.astro import plot as astroplot

x1 = [100, 200, 600, 1200]
y1 = [2000, 2100, 1400, 3050]

d1 = data.Data1D('oned', x1, y1)

plot1 = plot.DataPlot()
plot1.prepare(d1)
plot1.plot()
show(plot.backend.current_fig['all_axes'])

and a new tab opens up with a file-not-found error (in my case looking for file:///tmp/tmpg6_1hr2n.html but it's obviously a randomly-generated file.

Actually, the file exists but for some reason trying to display it fails. At first I thought it was a permissions error from jupyter lab, but trying "gio open /tmp/...html" failed in the same way. It's too late and wet for me to diagnose. Is firefox being protective of my /tmp/ directory (or something equally annoying to diagnose)? I think so, because if I "firefox /tmp/..." or "gio open /tmp/..." you get a file-not-found error, but if I manually open the file via file/open file it displays. Gah....

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>Bokeh Plot</title>
    <style>
      html, body {
        box-sizing: border-box;
        display: flow-root;
        height: 100%;
        margin: 0;
        padding: 0;
      }
    </style>
    <script type="text/javascript" src="https://cdn.bokeh.org/bokeh/release/bokeh-3.2.2.min.js"></script>
    <script type="text/javascript">
        Bokeh.set_log_level("info");
    </script>
  </head>
  <body>
    <div id="e0448a2b-9e27-4fb9-a01d-57a21641536b" data-root-id="p1063" style="display: contents;"></div>
  
    <script type="application/json" id="p1076">
      {"21276715-a5ae-4bcf-b18f-09fa6758ce03":{"version":"3.2.2","title":"Bokeh Application","roots":[{"type":"object","name":"GridPlot","id":"p1063","attributes":{"rows":null,"cols":null,"toolbar":{"type":"object","name":"Toolbar","id":"p1062","attributes":{"tools":[{"type":"object","name":"PanTool","id":"p1054"},{"type":"object","name":"WheelZoomTool","id":"p1055"},{"type":"object","name":"BoxZoomTool","id":"p1056","attributes":{"overlay":{"type":"object","name":"BoxAnnotation","id":"p1057","attributes":{"syncable":false,"level":"overlay","visible":false,"left_units":"canvas","right_units":"canvas","bottom_units":"canvas","top_units":"canvas","line_color":"black","line_alpha":1.0,"line_width":2,"line_dash":[4,4],"fill_color":"lightgrey","fill_alpha":0.5}}}},{"type":"object","name":"SaveTool","id":"p1061"},{"type":"object","name":"ResetTool","id":"p1059"},{"type":"object","name":"HelpTool","id":"p1060"}]}},"children":[[{"type":"object","name":"Figure","id":"p1033","attributes":{"x_range":{"type":"object","name":"DataRange1d","id":"p1034"},"y_range":{"type":"object","name":"DataRange1d","id":"p1035"},"x_scale":{"type":"object","name":"LinearScale","id":"p1065"},"y_scale":{"type":"object","name":"LinearScale","id":"p1066"},"title":{"type":"object","name":"Title","id":"p1040","attributes":{"text":"oned"}},"renderers":[{"type":"object","name":"GlyphRenderer","id":"p1071","attributes":{"data_source":{"type":"object","name":"ColumnDataSource","id":"p1067","attributes":{"selected":{"type":"object","name":"Selection","id":"p1068","attributes":{"indices":[],"line_indices":[]}},"selection_policy":{"type":"object","name":"UnionRenderers","id":"p1069"},"data":{"type":"map","entries":[["x",{"type":"ndarray","array":{"type":"bytes","data":"ZAAAAMgAAABYAgAAsAQAAA=="},"shape":[4],"dtype":"int32","order":"little"}],["y",{"type":"ndarray","array":{"type":"bytes","data":"0AcAADQIAAB4BQAA6gsAAA=="},"shape":[4],"dtype":"int32","order":"little"}]]}}},"view":{"type":"object","name":"CDSView","id":"p1072","attributes":{"filter":{"type":"object","name":"AllIndices","id":"p1073"}}},"glyph":{"type":"object","name":"Scatter","id":"p1070","attributes":{"size":{"type":"value","value":5},"fill_color":{"type":"value","value":"black"}}}}}],"toolbar":{"type":"object","name":"Toolbar","id":"p1041","attributes":{"tools":[{"id":"p1054"},{"id":"p1055"},{"id":"p1056"},{"type":"object","name":"SaveTool","id":"p1058"},{"id":"p1059"},{"id":"p1060"}]}},"toolbar_location":null,"left":[{"type":"object","name":"LinearAxis","id":"p1049","attributes":{"ticker":{"type":"object","name":"BasicTicker","id":"p1050","attributes":{"mantissas":[1,2,5]}},"formatter":{"type":"object","name":"BasicTickFormatter","id":"p1051"},"axis_label":"y","major_label_policy":{"type":"object","name":"AllLabels","id":"p1052"}}}],"below":[{"type":"object","name":"LinearAxis","id":"p1044","attributes":{"ticker":{"type":"object","name":"BasicTicker","id":"p1045","attributes":{"mantissas":[1,2,5]}},"formatter":{"type":"object","name":"BasicTickFormatter","id":"p1046"},"axis_label":"x","major_label_policy":{"type":"object","name":"AllLabels","id":"p1047"}}}],"center":[{"type":"object","name":"Grid","id":"p1048","attributes":{"axis":{"id":"p1044"}}},{"type":"object","name":"Grid","id":"p1053","attributes":{"dimension":1,"axis":{"id":"p1049"}}}]}},0,0]]}}]}}
    </script>
    <script type="text/javascript">
      (function() {
        const fn = function() {
          Bokeh.safely(function() {
            (function(root) {
              function embed_document(root) {
              const docs_json = document.getElementById('p1076').textContent;
              const render_items = [{"docid":"21276715-a5ae-4bcf-b18f-09fa6758ce03","roots":{"p1063":"e0448a2b-9e27-4fb9-a01d-57a21641536b"},"root_ids":["p1063"]}];
              root.Bokeh.embed.embed_items(docs_json, render_items);
              }
              if (root.Bokeh !== undefined) {
                embed_document(root);
              } else {
                let attempts = 0;
                const timer = setInterval(function(root) {
                  if (root.Bokeh !== undefined) {
                    clearInterval(timer);
                    embed_document(root);
                  } else {
                    attempts++;
                    if (attempts > 100) {
                      clearInterval(timer);
                      console.log("Bokeh: ERROR: Unable to run BokehJS code because BokehJS library is missing");
                    }
                  }
                }, 10, root)
              }
            })(window);
          });
        };
        if (document.readyState != "loading") fn();
        else document.addEventListener("DOMContentLoaded", fn);
      })();
    </script>
  </body>
</html>

[*] EDITED TO ADD this is version 4.0.6 of jupyterlab (although there's a bunch of other related components), in case it's a jupyter permissions issue [but I don't think it is, at least not completely]

@wmclaugh
Copy link
Contributor

Plotting and displaying in my browser (chrome on macOS 13) with the Bokeh back end works. Accessing help on the plot object shows a possible discrepancy:

plot.set_backend('BokehBackend')
plot1 = plot.DataPlot()
help(plot1)
<... screen output trimmed ...>
| >>> dplot.xlabel = 'length'
| >>> dplot.ylabel = 'data values'
| >>> dplot.plot(marker=' ', linestyle='dashed')

Note that the help example has the marker is set to a space (ascii char 0x20). However, using that marker setting results in the following error message (plot displays):

plot1.plot(marker=' ', linestyle='dashed')
show(plot.backend.current_fig['all_axes'])
ERROR:bokeh.core.validation.check:E-1001 (BAD_COLUMN_NAME): Glyph refers to nonexistent column name. This could either be due to a misspelling or typo, or due to an expected column being missing. : marker=' ' [no close matches] {renderer: GlyphRenderer(id='p1124', ...)}

Setting the marker to the empty string generates a plot without markers and does not generate the warning.

@wmclaugh
Copy link
Contributor

Where is the temp file cleanup handled? Deleting the objects doesn't appear to clean up the tmp files and after exiting out of the python and closing the bokeh plot tabs, I can still view the plot image by accessing the /tmp/*.html file.

@hamogu
Copy link
Contributor Author

hamogu commented Sep 19, 2023

I don't actually know where clean-up is handled - I consider that out of scope for Sherpa. Sherpa does not specify where to put any file (or to have any file at all), that's in the bokeh layer.
I understand that that's not a good answer from a CS stand-point, but we're using bokeh as intended here. What we can (and have to!) is to work on the documentation for how to use bokeh by summarizing the most important points and linking to the bokeh docs for more details. For example, a common pattern in bokeh is to specifically set a file name:

https://docs.bokeh.org/en/latest/docs/first_steps/first_steps_7.html

In that case, it'll keep overwriting that one specific file name for every new plot you make.

@hamogu
Copy link
Contributor Author

hamogu commented Sep 19, 2023

@DougBurke Can you try a plain bokeh plot (no sherpa involved) and see if that works. If not, then we can fairly confidently say that it's jupyterlab.

from bokeh.plotting import figure, show
# prepare some data
x = [1, 2, 3, 4, 5]
y = [6, 7, 2, 4, 5]
# create a new plot with a title and axis labels
p = figure(title="Simple line example", x_axis_label='x', y_axis_label='y')
# add a line renderer with legend and line thickness to the plot
p.line(x, y, legend_label="Temp.", line_width=2)
# show the results
show(p)

From: https://docs.bokeh.org/en/latest/docs/first_steps/first_steps_1.html (see there for more details). It might also be specific to jupyerlab, as opposed to the classic Jupyter notebook that I use (though I thought Project Jupyter has merged both projects now that the notebook today is just a specific UI on top of jupyterlab) - you might try https://github.com/bokeh/jupyter_bokeh

@DougBurke
Copy link
Contributor

FYI I'm not sure how much time I have today for bokeh.

@DougBurke
Copy link
Contributor

Now that #1382 has been merged can we have this rebased, to make it easier to review? [not that I'm likely to do it today]

@wmclaugh
Copy link
Contributor

Plotting and displaying in my browser (chrome on macOS 13) with the Bokeh back end works. Accessing help on the plot object shows a possible discrepancy:

plot.set_backend('BokehBackend')
plot1 = plot.DataPlot()
help(plot1)
<... screen output trimmed ...>
| >>> dplot.xlabel = 'length'
| >>> dplot.ylabel = 'data values'
| >>> dplot.plot(marker=' ', linestyle='dashed')

Note that the help example has the marker is set to a space (ascii char 0x20). However, using that marker setting results in the following error message (plot displays):

plot1.plot(marker=' ', linestyle='dashed')
show(plot.backend.current_fig['all_axes'])
ERROR:bokeh.core.validation.check:E-1001 (BAD_COLUMN_NAME): Glyph refers to nonexistent column name. This could either be due to a misspelling or typo, or due to an expected column being missing. : marker=' ' [no close matches] {renderer: GlyphRenderer(id='p1124', ...)}

Setting the marker to the empty string generates a plot without markers and does not generate the warning.

I believe this comes from line 1276 of sherpa/plot/init.py in Class DataPlot. Since both existing backends (bokeh and pylab) take the empty string for no markers, perhaps the space should be omitted.

>>> dplot.plot(**marker=' '**, linestyle='dashed')

@DougBurke
Copy link
Contributor

I'm re-running the jobs because the error was a bizarre conda issue unrelated to this code.

@hamogu
Copy link
Contributor Author

hamogu commented Sep 21, 2023

I'm activating most of the tests for bokeh, but because of #1902 at least three of them fail (and since we parameterize, the actual number of failures is larger).
Depending on how far we get with this in the next two days, there is probably some say to xfail them (but it's annoying because of the double parameterization using both a pytxt fixture and a pytest.mark.parameterize in the same test; alternatively, I'll just put in a if plot.backend.name == 'BokehBackend': return (which will have the same effect as an xfail) and note that in #1902 so we don't forget to take it out later.

@hamogu hamogu force-pushed the bokeh_class branch 2 times, most recently from 7b20146 to 624e37f Compare September 21, 2023 12:02
@DougBurke
Copy link
Contributor

In reference to #1899 (comment) - it's probably because I'm using ubuntu, and hence firefox via a snap (so would be similar possibilities about flatpak version) which probably can't see /tmp/ ...

This is what clued me in to the problem, https://askubuntu.com/questions/1389798/how-to-correctly-configure-snapd-firefox-to-open-local-html-file-generated-by but it's not realy helped me with a solution.

This is much older, and so may be wrong now, but it's basically saying I have to bind mount the /tmp directory which is just insane (I can understand the snap restriction, and that this problem is not bokeh or snap's "fault", but it is a UX fail...

Now I just need to know what I can point bokeh.io.output_file('some/other/path/firefox/can/read.html') to... I was able to

mkdir ~/tmp

and then

bokeh.io.output_file('/home/dburke/tmp/tmp_bokeh.html')

and the bokeh-only code created a plot.

@DougBurke
Copy link
Contributor

So is the idea that we get #1906 merged, so this can then be merged into this (or rebased,or whatever) and everything will be copaceptic ;-)

This is a fully working implementation, that could be merged as is.
However, there are a few UI improvements that might be worthwile, e.g. there is no color cycline and the bokeh.plot.show could do with a nice wrapper in sherpa.

fixes sherpa#499
Note that some tests that produce images fail due to sherpa#1902.
@DougBurke DougBurke removed the note:needs SDS testing PR requires SDS testing and feedback label Sep 27, 2023
@hamogu hamogu force-pushed the bokeh_class branch 2 times, most recently from cfb3b80 to 03e8cb7 Compare September 27, 2023 21:08
Also includes some unrelated typos
Copy link
Contributor

@DougBurke DougBurke left a comment

Choose a reason for hiding this comment

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

THere's a print line I've noted but other than that lgtm.

@hamogu
Copy link
Contributor Author

hamogu commented Sep 28, 2023

Removing that spare print line will also restart the tests...

@DougBurke
Copy link
Contributor

Again the test failure is not related to this code (*).


  • technically there could be some relation, but I do not see how, and I feel I've seen this problem on other PRs but can't guarantee it.

@hamogu
Copy link
Contributor Author

hamogu commented Oct 2, 2023

Note that the test that fails here is "Pip CI/Linux Build (w/o XSPEC, Python 3.11) (https://github.com/sherpa/sherpa/actions/runs/6341413247/job/17224999177?pr=1899). That's the only one that I've added bokeh, to (I think) because I did not want to drive up our testing times for something that's still a little experimental. Also, bokeh is pretty independent of Sherpa, in the sense that we don't build C code are binaries that have anything to do with bokeh.

So, it seemed like a reasonable compromise for the time being.

I have a solution to that failure in hamogu#6, where "Pip CI/Linux Build (w/o XSPEC, Python 3.11)" passes: https://github.com/hamogu/sherpa/actions/runs/6385025927/job/17328888999?pr=6

I will move that code into this PR in the next few minutes, but we will now get a different failure in some other build that has noting to do with this PR, but that @DougBurke reported this morning and that probably has something to do with some dependency being upgraded on conda-forge. (We might have to find with one and pin it for the time being).

@wmclaugh wmclaugh merged commit b3ac913 into sherpa:main Oct 3, 2023
12 of 17 checks passed
@hamogu hamogu deleted the bokeh_class branch October 5, 2023 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants