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

PR: Add new "Plotting library" option, defaulting to Matplotlib (Variable Explorer) #20075

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

PierreRaybaut
Copy link
Contributor

Description of Changes

Added new option "Plotting library" to Variable Explorer plugin:
Capture d’écran 2022-11-17 105527

This allows to choose one of the compatible libraries (currently: matplotlib or guiqwt) for plotting data from the Variable Explorer. Until this PR, default plotting library was guiqwt (if installed) or matplotlib (if guiqwt was not found). This option works for the %varexp magic command (i.e. within currently running IPython kernel) and for nested arrays (i.e. within Spyder main process).

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

@ccordoba12 ccordoba12 changed the title Variable Explorer: new "Plotting library" option (default: matplotlib) PR: Add new "Plotting library" option, defaulting to Matplotlib (Variable Explorer) Nov 24, 2022
@ccordoba12 ccordoba12 changed the base branch from master to 5.x November 24, 2022 16:43
@ccordoba12 ccordoba12 changed the base branch from 5.x to master November 24, 2022 16:43
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Hey @PierreRaybaut, thanks a lot your first contribution after a long time! This is my initial review for you.

spyder/plugins/plots/widgets/figurebrowser.py Show resolved Hide resolved
spyder/plugins/variableexplorer/confpage.py Outdated Show resolved Hide resolved
Comment on lines 38 to 42
def get_default_plotlib():
"""Return default plotting library"""
names = get_available_plotlibs()
if names is not None:
return names[0]
Copy link
Member

Choose a reason for hiding this comment

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

This function returns None if no library is available, which I think wouldn't look good in our Preferences. So, perhaps it should return Matplotlib?

Copy link
Contributor Author

@PierreRaybaut PierreRaybaut Nov 25, 2022

Choose a reason for hiding this comment

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

Actually that's not a problem because if there is no library available, the combo box won't be displayed in Preferences: combo box is added to Preferences page only if plotlib.AVAILABLE_PLOTLIBS is not empty. But I realize that it's not a good idea because even if plotlib.AVAILABLE_PLOTLIBS is empty, that does not mean that the IPython kernel won't have the library installed. So I'm rolling back on this.

And there is another problem: I've just did some tests and apparently Spyder Preferences do not handle well dynamic choices in combo boxes: if the choices are not the same as the previous run, the displayed choice in Preferences page will be changed but won't be considered as a new value when validating the dialog box. In other words, in that last case and if the list of choices is reduced to a single choice, it won't be possible to select the one and only choice available by validating the Preferences page...

So, I'm rolling back and proposing a static list for plotting libraries in the Preferences page. This should have some impact on other modules. I'll see about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See 7adeece


# Defining compatible plotting libraries
# (default library is the first available, see `get_default_plotlib`)
LIBRARIES = ("matplotlib", "guiqwt")
Copy link
Member

Choose a reason for hiding this comment

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

I think these names should be capitalized to look good in our Preferences. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both library logos display their names in all lowercase, so I would suggest not bothering with capitalizing them, hence simplifying code (key == value, as for the combo box management).

Comment on lines 1147 to 1150
QMessageBox.critical(self, _( "Plot"),
_("<b>Unable to plot data.</b>"
"<br><br>Error message:<br>%s"
) % str(error))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
QMessageBox.critical(self, _( "Plot"),
_("<b>Unable to plot data.</b>"
"<br><br>Error message:<br>%s"
) % str(error))
QMessageBox.critical(
self,
_( "Plot"),
_("<b>Unable to plot data.</b><br><br>"
"The error message was:<br>%s") % str(error)
)

Improve formatting and message a bit.

Copy link
Contributor Author

@PierreRaybaut PierreRaybaut Nov 25, 2022

Choose a reason for hiding this comment

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

Regarding code formatting, you really should consider using Black and force everyone contributing to do so. At work, all my teams are using Black together with isort with proper configuration (I gradually imposed this on all projects since 2018). CodraFT is an example of project following standard guidelines in terms of code quality.

Using (and forcing all contributors to use) Black saves time, increases code quality and helps focusing on real code changes (instead of being annoyed with code appearance).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I push this further? (by integrating Black into Spyder for example?)

spyder/widgets/collectionseditor.py Outdated Show resolved Hide resolved
spyder/widgets/collectionseditor.py Outdated Show resolved Hide resolved
spyder/widgets/collectionseditor.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 added this to the v6.0alpha1 milestone Nov 24, 2022
@ccordoba12
Copy link
Member

Note: I'm marking this for Spyder 6 because it requires changes both in the kernel and Spyder to work. Those kind of changes are released on minor or major versions (e.g. 5.5.0 or 6.0.0), but the core team decided that we're not going to have more minor versions to try to release Spyder 6 sooner.

PierreRaybaut and others added 7 commits November 25, 2022 09:10
Improve formatting and message a bit

Co-authored-by: Carlos Cordoba <[email protected]>
Improve formatting and message a bit

Co-authored-by: Carlos Cordoba <[email protected]>
Improve formatting and message a bit

Co-authored-by: Carlos Cordoba <[email protected]>
…nels.git --branch=improving_guiqwt_integration_kernel --update --force external-deps/spyder-kernels

subrepo:
  subdir:   "external-deps/spyder-kernels"
  merged:   "6c5418b28"
upstream:
  origin:   "https://github.com/PierreRaybaut/spyder-kernels.git"
  branch:   "improving_guiqwt_integration_kernel"
  commit:   "6c5418b28"
git-subrepo:
  version:  "0.4.5"
  origin:   "???"
  commit:   "???"
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha1, v6.0alpha2 Jun 8, 2023
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha2, v6.0alpha3 Jul 29, 2023
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha3, v6.0beta1 Nov 17, 2023
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha4, v6.0beta1 Feb 6, 2024
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha5, v6.0beta1 Mar 12, 2024
@ccordoba12 ccordoba12 removed this from the v6.0beta1 milestone May 10, 2024
@ccordoba12 ccordoba12 added this to the v6.0beta2 milestone May 10, 2024
@ccordoba12 ccordoba12 modified the milestones: v6.0beta2, v6.0beta3 Jun 17, 2024
@ccordoba12 ccordoba12 modified the milestones: v6.0beta3, v6.1.0 Jul 7, 2024
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.

2 participants