Skip to content

Lazy load plotting libraries#209

Merged
lewisjared merged 3 commits intomasterfrom
lazy-load-plotting
Oct 26, 2022
Merged

Lazy load plotting libraries#209
lewisjared merged 3 commits intomasterfrom
lazy-load-plotting

Conversation

@lewisjared
Copy link
Copy Markdown
Collaborator

@lewisjared lewisjared commented Oct 24, 2022

Pull request

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Example added (either to an existing notebook or as a new notebook, where applicable)
  • Description in CHANGELOG.rst added

@znicholls Do you have any strong feelings against lazy importing the plotting libraries? They add >1s of import time which is very obvious when running small python scripts without any plotting. This in combination with #208 reduces scmdata's import time to ~300ms down from >2s.

This pattern is used in other libraries to avoid always importing matplotlib.pyplot (https://github.com/pydata/xarray/blob/7379923de756a2bcc59044d548f8ab7a68b91d4e/xarray/plot/dataarray_plot.py#L979).

FYI: There is an interesting pattern to allow importing units for type checking only by optionally importing if typing.TYPE_CHECKING is True https://github.com/pydata/xarray/blob/7379923de756a2bcc59044d548f8ab7a68b91d4e/xarray/plot/dataarray_plot.py#L979

@znicholls
Copy link
Copy Markdown
Collaborator

Looks good.

Yep type checking stuff is a good pattern I often forget about too

@lewisjared
Copy link
Copy Markdown
Collaborator Author

Cool I'll clean this up

Base automatically changed from unit-registry to master October 25, 2022 10:48
@lewisjared lewisjared marked this pull request as ready for review October 25, 2022 11:16
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 25, 2022

Codecov Report

Base: 95.83% // Head: 95.82% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (e6f9ff8) compared to base (97342c0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
- Coverage   95.83%   95.82%   -0.02%     
==========================================
  Files          23       23              
  Lines        2090     2084       -6     
  Branches      371      370       -1     
==========================================
- Hits         2003     1997       -6     
  Misses         69       69              
  Partials       18       18              
Impacted Files Coverage Δ
src/scmdata/ops.py 99.50% <100.00%> (ø)
src/scmdata/plotting.py 95.09% <100.00%> (-0.28%) ⬇️
src/scmdata/run.py 95.94% <100.00%> (ø)
src/scmdata/testing.py 70.58% <100.00%> (ø)
src/scmdata/units.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lewisjared
Copy link
Copy Markdown
Collaborator Author

@znicholls No major changes on clean up so I'll merge this and do a minor release unless I hear a scream

@lewisjared
Copy link
Copy Markdown
Collaborator Author

Waiting for hgrecco/pint-pandas#145 to be resolved before a release


@patch("scmdata.plotting.has_matplotlib", False)
def test_no_matplotlib(scm_run):
def hide_import(import_name):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is this pattern?

@znicholls
Copy link
Copy Markdown
Collaborator

Waiting for hgrecco/pint-pandas#145 to be resolved before a release

Can you tell what pint has changed and why everything has now exploded?

@lewisjared
Copy link
Copy Markdown
Collaborator Author

Can you tell what pint has changed and why everything has now exploded?

From a look at the changelog there was a refactor into "Facets" (hgrecco/pint#1466). I haven't dived into this. It broke something in primap related to its unit registry so we might have to check openscm_units (hgrecco/pint#1631)

@lewisjared
Copy link
Copy Markdown
Collaborator Author

MIght be better to pin to <0.2 for the next release and then loosen once we do some testing

@znicholls
Copy link
Copy Markdown
Collaborator

znicholls commented Oct 26, 2022 via email

This is a hotfix until pint_pandas releases a new version
@lewisjared lewisjared merged commit b04ae2e into master Oct 26, 2022
@lewisjared lewisjared deleted the lazy-load-plotting branch October 26, 2022 04:25
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