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

PERF: create unit menu on-demand for QLineEdit #1027

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

klauer
Copy link
Collaborator

@klauer klauer commented Aug 29, 2023

Background

We noticed that a bunch of QMenu instances were in our typhos test suite at the top-level (QApplication.instance().topLevelWidgets()).
While tangential to the issue we were investigating in that repository, it seems unnecessary for PyDM to create these at __init__ time.

This PR

This PR creates the unit conversion menu on-demand.

Testing

  • Test suite was updated
  • Quickly checked the example to see if it still works:
image

Copy link
Collaborator

@YektaY YektaY left a comment

Choose a reason for hiding this comment

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

looks great!

@YektaY YektaY merged commit bb77393 into slaclab:master Aug 29, 2023
18 checks passed
@klauer klauer deleted the perf_unit_conversion branch August 29, 2023 20:26
@mattgibbs
Copy link
Collaborator

I think I remember the library that supplied the units for this being some big unwieldy thing that took a long time to import, and I suspect it effectively gets imported at launch time, because it lives in utilities. We should maybe move that import to happen the first time this menu is made…

@klauer
Copy link
Collaborator Author

klauer commented Aug 30, 2023

I think I remember the library that supplied the units for this being some big unwieldy thing that took a long time to import, and I suspect it effectively gets imported at launch time, because it lives in utilities. We should maybe move that import to happen the first time this menu is made…

I think you might be remembering from a different project. PyDM uses scipy.constants (which for me, at least, imports pretty quickly). On our side, the library pint definitely does slow things down so we import on-demand as you've suggested.

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.

3 participants