gafusion / omas

Ordered Multidimensional Array Structure
http://gafusion.github.io/omas
MIT License
30 stars 14 forks source link

Incompatible with matplotlib >= 3.6.0 #266

Closed hassec closed 10 months ago

hassec commented 10 months ago

The current code doesn't seem to work with more recent matplotlib versions.

Reproducer:

from omas import ODS

o = ODS().sample(ntimes=2)
o.plot_equilibrium_summary()

Error:

AttributeError                            Traceback (most recent call last)
Cell In[4], line 1
----> 1 e.plot_equilibrium_summary()

File ~/software/pyenv/versions/3.10.12/envs/mosaic/lib/python3.10/site-packages/omas/omas_plot.py:986, in equilibrium_summary(ods, time_index, time, fig, ggd_points_triangles, **kw)
    984         time_index = time_index[0]
    985     else:
--> 986         return ods_time_plot(
    987             equilibrium_summary, ods, time_index, time, fig=fig, ggd_points_triangles=ggd_points_triangles, ax=axs, **kw
    988         )
    990 ax = cached_add_subplot(fig, axs, 1, 3, 1)
    991 contour_quantity = kw.pop('contour_quantity', 'rho_tor_norm')

File ~/software/pyenv/versions/3.10.12/envs/mosaic/lib/python3.10/site-packages/omas/omas_plot.py:518, in ods_time_plot(ods_plot_function, ods, time_index, time, **kw)
    516 stime.on_changed(update)
    517 for time0 in time:
--> 518     axtime.axvline(time0, color=['r', 'y', 'c', 'm'][stime.cnt - 2])
    519 return {'stime': (stime, axtime), 'ax': axs}

AttributeError: 'Slider' object has no attribute 'cnt'

Additional info

The code in question is: https://github.com/gafusion/omas/blob/0cd6e21bb135ae6f1cb64d57e252c5a3c4e480ca/omas/omas_plot.py#L517-L519

which accesses Slider.cnt which was deprecated in version 3.4.0 and then removed in 3.6.0.

orso82 commented 10 months ago

Thank you for pointing that out @hassec ! Do you know what it was substituted with?

hassec commented 10 months ago

@orso82 AFAIU it was removed and didn't get an equivalent replacement.

orso82 commented 10 months ago

Perhaps slider.val ? can you please try it out?

>>> import matplotlib.widgets
>>> from matplotlib import pyplot
>>> ax=pyplot.ax()
>>> ax=pyplot.gca()
>>> slider = matplotlib.widgets.Slider(ax, "bla", 1, 10)
>>> slider.<TAB>
slider.active               slider.connect_event(       slider.eventson             slider.orientation          slider.slidermax            slider.valinit              slider.vline
slider.ax                   slider.disconnect(          slider.get_active()         slider.poly                 slider.slidermin            slider.valmax
slider.canvas               slider.disconnect_events()  slider.ignore(              slider.reset()              slider.track                slider.valmin
slider.closedmax            slider.drag_active          slider.label                slider.set_active(          slider.val                  slider.valstep
slider.closedmin            slider.drawon               slider.on_changed(          slider.set_val(             slider.valfmt               slider.valtext
>>> slider.val
1
>>> slider.set_val(2)
>>> slider.val
2
hassec commented 10 months ago

slider.val is the current value, and set_val sets that value.

Slider.cnt was a property that gave the quantity of registered callbacks as far as I understand. And while deprecated, it was implemented like:

https://github.com/matplotlib/matplotlib/blob/d04c8de4a89ab756a137ef146e8542e81041909f/lib/matplotlib/widgets.py#L156-L157

in 3.4 and 3.5 versions before it was removed.

I can try and have a look later to see if I an find a workaround that still works with both versions.

orso82 commented 10 months ago

Yes, you're right. That would be great. Worse case we can always simplify with:

for time0 in time: 
     axtime.axvline(time0, color='r')
hassec commented 10 months ago

Hi @orso82,

sorry for the slow action, I've not had any time to check into this more until today. I'm not sure what the best fix would be because I'm not sure I really understand what

     axtime.axvline(time0, color=['r', 'y', 'c', 'm'][stime.cnt - 2]) 

is used for. I checked the tests but couldn't find a use case where stime.cnt has any value other than 2. When would that value be higher?

Do you have an example?

orso82 commented 10 months ago

That would mark with different colors the times at which the data of multiple time dependent quantities are available (eg. equilibrium and ECH)

image

hassec commented 10 months ago

@orso82 do you have an example minimal reproducer or some code in OMFIT that I could turn into a test?

orso82 commented 10 months ago

Not really, not now. I'll simply set it to axtime.axvline(time0, color='r'), which will be sufficient for most use cases.

orso82 commented 10 months ago

running on matplotlib >= 3.6.0 is definitely more important

hassec commented 10 months ago

Thanks @orso82 for implementing the workaround :+1:

Is there already a rough ETA for when a new release OMAS will be made that includes this and the fixes since the last release?

orso82 commented 10 months ago

We can release any time, but perhaps I'll let @smithsp @bechtt @jmcclena comment if we should squeeze in some of their latest PRs

hassec commented 10 months ago

That would be fantastic, thanks @orso82! If we could also get #262 into that release, it would mean we can remove all omas patches we currently have in place in our projects :)

torrinba commented 10 months ago

My PR is ready (just waiting for Joey's approval) and also requires updating the OMAS version used in OMFIT. It can come later if this is more pressing though

jmcclena commented 10 months ago

@orso82 I finished squeezing all my PRs in.

orso82 commented 10 months ago

https://pypi.org/project/omas/0.92.0