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

Add ruler tool in plots #291

Merged
merged 9 commits into from Jun 28, 2021
Merged

Conversation

erwanp
Copy link
Member

@erwanp erwanp commented Jun 18, 2021

Description

  • Add a Cursor in Spectrum.plot() (as was in plot_diff already)
  • (experimental) Add an optnioal Ruler tool in Spectrum.plot(show_ruler=True) and plot_diff(show_ruler=True)

image

Still some tiny problems with Ruler tool :

  • once initialized, it resets the zoom the first time. I don't know why.

@erwanp erwanp added the post-process Related to the post-processing (ex: methods of Spectrum objects) label Jun 18, 2021
@erwanp erwanp added this to the 0.9.30 milestone Jun 18, 2021
@erwanp erwanp changed the title Add/ruler tool in plots Add ruler tool in plots Jun 18, 2021
@erwanp erwanp requested a review from minouHub June 18, 2021 20:59
@erwanp erwanp added the interface not related to the physics of the code label Jun 18, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2021

Codecov Report

Merging #291 (8a8a527) into develop (4ebfa27) will decrease coverage by 0.72%.
The diff coverage is 18.69%.

@@             Coverage Diff             @@
##           develop     #291      +/-   ##
===========================================
- Coverage    75.24%   74.51%   -0.73%     
===========================================
  Files          119      121       +2     
  Lines        14273    14627     +354     
===========================================
+ Hits         10740    10900     +160     
- Misses        3533     3727     +194     

@erwanp
Copy link
Member Author

erwanp commented Jun 19, 2021

@CorentinGrimaldi could easily be added to Fitroom too (it's just an add_ruler() function) !

@minouHub
Copy link
Collaborator

Hi Erwan,
Pulled your branch. It works. However, I cannot zoom and then use the ruler.

Also got a lot of (intended?) warnings:

<matplotlib.lines.Line2D at 0x21bd051ccd0>e:\python\radis\radis\tools\plot_tools.py:199: MatplotlibDeprecationWarning: Setting the line's pick radius via set_picker is deprecated since 3.3 and will be removed two minor releases later; use set_pickradius instead.
  (self._marker_a,) = self.ax.plot((x0, y0), **used_markerprops)
e:\python\radis\radis\tools\plot_tools.py:200: MatplotlibDeprecationWarning: Setting the line's pick radius via set_picker is deprecated since 3.3 and will be removed two minor releases later; use set_pickradius instead.
  (self._marker_b,) = self.ax.plot((x0, y0), **used_markerprops)
e:\python\radis\radis\tools\plot_tools.py:201: MatplotlibDeprecationWarning: Setting the line's pick radius via set_picker is deprecated since 3.3 and will be removed two minor releases later; use set_pickradius instead.
  (self._marker_c,) = self.ax.plot((x0, y0), **used_markerprops)

@erwanp
Copy link
Member Author

erwanp commented Jun 21, 2021

  • The first warning is unfortunate. It is already suppressed in RADIS, but it only works with matplotlib>=3.4 which itself requires Python 3.7+. (Using the "new" toolbar manager still triggers warnings referring to V. 1.5 / 2.1 matplotlib/matplotlib#15284)
    I initially enforced matplotlib>=3.4 but many Radis users probably still have Python 3.5 or 3.6 and may not want to upgrade yet ; they better receive the warnings.

  • I think I fixed the other 2 warnings.

  • When you say you cannot Zoom & user the ruler : can't you unselect the Zoom and then use the ruler ?

  • I also noticed that the ruler is not enabled on the first plot ; can you confirm this ?

@minouHub
Copy link
Collaborator

minouHub commented Jun 27, 2021

I also noticed that the ruler is not enabled on the first plot ; can you confirm this

Yes, that's quick funny. It works the second time.

First run:

e:\python\radis\radis\tools\plot_tools.py:561: UserWarning: Couldn't add Ruler tool (still an experimental feature in RADIS : please report the error !)
  warn(
  • no ruler

Second run:

Treat the new Tool classes introduced in v1.5 as experimental for now, the API will likely change in version 2.1 and perhaps the rcParam as well
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\spectrum\spectrum.py:1498: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  fig = plt.figure(nfig)
e:\python\radis\radis\tools\plot_tools.py:527: UserWarning: The new Tool classes introduced in v1.5 are experimental; their API (including names) will likely change in future versions.
  super().__init__(*args, **kwargs)
  • many warnings
  • (minor) If I zoom or move the plot first, clicking on the ruler does not unselect the drag/zoom button. It took me 5 minutes to realize this is why I could not use the ruler after a zoom ^^

@erwanp
Copy link
Member Author

erwanp commented Jun 27, 2021

Ok so none of this looks production-ready.
What about adding it, but as an experimetanl option in the Radis.json, False by default ?.

@minouHub
Copy link
Collaborator

I think that's a very good idea to add a False/True somewhere. This way, I can use the ruler and it leaves the mainstream radis clean for other users. However, I expected this solution to be too complicated as it might mess up with the matplotlib version?

Copy link
Collaborator

@minouHub minouHub left a comment

Choose a reason for hiding this comment

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

Pulled!
Alright for me :)

@erwanp erwanp merged commit 7b3a447 into radis:develop Jun 28, 2021
@erwanp erwanp mentioned this pull request Jul 6, 2021
@erwanp erwanp deleted the add/ruler-tool-in-plots branch July 7, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interface not related to the physics of the code post-process Related to the post-processing (ex: methods of Spectrum objects)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants