fix broken plotting functions and update for Bokeh 3.x#198
fix broken plotting functions and update for Bokeh 3.x#198PrasannaPal21 wants to merge 6 commits intoioos:mainfrom
Conversation
|
|
ioos_qc/plotting.py
Outdated
| "plot_width": 600, | ||
| "plot_height": 200, | ||
| "ncols": 2, | ||
| **kwargs, | ||
| } | ||
|
|
||
| if "plot_width" in kwargs: | ||
| kwargs.pop("plot_width") | ||
| if "plot_height" in kwargs: | ||
| kwargs.pop("plot_height") |
There was a problem hiding this comment.
Can you explain these changes with your own words, not AI, and what problem are you trying to solve here?
There was a problem hiding this comment.
plot_width and plot_height were deprecated in bokeh 2.4.0 and were completely removed in 3.0.0. so it was causing the unexpected keyword argument error when we use the arbitrary **kwargs into the plotting function.
kwargs is a dictionary, so before we call gridplot, removing(popping) these( plot_width and plot_height ), if present, we prevent the crashes.
these were replaced by width and height in the later version of bokeh.
There was a problem hiding this comment.
So, why not update the kw here? Poping them hides the wrong syntax. It smells of an AI solution that just tries to solve the error rather than understand the problem.
There was a problem hiding this comment.
I honestly just googled and saw that those were completely removed so my first thought was just to quickly pop them out of the kwargs so that gridplot would not crash.
There was a problem hiding this comment.
should I remap it kwarg["width"] = kwarg.pop("plot_width")
There was a problem hiding this comment.
Why hide them or renamed them when they were removed upstream? What is the best course of action for something that was remove? A clean API or some code magic that hides it?
ioos_qc/streams.py
Outdated
| def data(self, stream_id=None): | ||
| if stream_id is not None and isinstance(self.inp, dict) and stream_id in self.inp: | ||
| return self.inp[stream_id] |
There was a problem hiding this comment.
Please explain these changes and the jump in cyclomatic complexity here? Can this be simpler?
There was a problem hiding this comment.
previous data() method was returning the entire self.inp object. but if the inp was a dictionary with multiple variables, it would return whole dictionary instead of the specific variables which we requested. this was causing bokeh to crash when plotting.
I modified it so that if a dictionary is provided we return only the requested variable's data.
I am not getting the cyclomatic complexity thing, I'll study this and then try to find out if there is a better way to do this.
There was a problem hiding this comment.
cyclomatic complexity
Before it only returned the input. Anything downstream could filter this input that what they need. Now you add a new variable, check its value, check the input type, and check if the new variable is a key in the input. That is the extra complexity added.
Tip: How is this function used? What can be done in the use case to reduce that complexity?
|
hey @ocefpaf please take a look and guide if I am missing again something. |
|
Sorry for the delay in response. Understood that if those are deprecated by bokeh itself we dont have to take of them. currently reviewing the tests before requesting the review. |
2 participants
bokeh_multi_var()was callingbokeh_plot_var()with 7 args butbokeh_plot_var()only accepted 6 — added the missingvar_nameparameter. Also addedtimetobokeh_plot()since it was missing.Other fixes in plotting.py:
circle() → scatter()(deprecated in Bokeh 3.4)removed
plot_width/plot_heightfromgridplot()kwargs (removed in Bokeh 3.x)NumpyStream.data()didn't take astream_id, so plotting with NumpyStream would crash. Added optional stream_id support.