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

Recognize DARTS_CONFIGURE_MATPLOTLIB=0 #924

Merged
merged 2 commits into from Apr 23, 2022

Conversation

zgana
Copy link
Contributor

@zgana zgana commented Apr 20, 2022

Fixes #892.

Summary

This PR makes it possible to import darts without changing the global matplotlib configuration.

  • By default, darts still applies matplotlib configurations
  • If the environment variable DARTS_CONFIGURE_MATPLOTLIB=0 is set, then darts.u8plots_mplstyle is still defined but not applied.

Other Information

Normally it would be best to add tests of the new behavior, but it's difficult to test behavior in the root __init__.py. Both the pre-existing behavior and the adjustment here would be easier to test if the plotting configuration were factored out into some helper module. That would be a larger (though still fairly small) change.

A larger question would be: should darts change the global matplotlib configuration by default? Making this opt-in would obviously cause an aesthetic mismatch for anyone following documentation produced prior to that change. But it would arguably be cleaner (and more testable) to require e.g. darts.apply_u8plots_mplstyle() or otherwise respect users' matplotlibrc.

* By default, darts still applies matplotlib configurations
* If the environment variable DARTS_CONFIGURE_MATPLOTLIB=0 is set, then
darts.u8plots_mplstyle is still defined but not applied
Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

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

Thanks for this, LGTM.
I agree with your comment that opting-in using something along the lines of darts.apply_u8plots_mplstyle() would be better eventually. Modifying the global config is not great. We could modify TimeSeries.plot() in order to still apply some style in those plots only, maybe.

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2022

Codecov Report

Merging #924 (02aaad5) into master (e3f3309) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #924   +/-   ##
=======================================
  Coverage   91.70%   91.70%           
=======================================
  Files          74       74           
  Lines        7402     7404    +2     
=======================================
+ Hits         6788     6790    +2     
  Misses        614      614           
Impacted Files Coverage Δ
darts/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3f3309...02aaad5. Read the comment docs.

@zgana
Copy link
Contributor Author

zgana commented Apr 21, 2022

Thanks @hrzn! I just realized I did not add an entry to the change log. Should I push another commit with an entry under "Unreleased"?

@hrzn
Copy link
Contributor

hrzn commented Apr 21, 2022

Yes that'd be great - thanks!

@hrzn
Copy link
Contributor

hrzn commented Apr 23, 2022

Thank you @zgana, merging now, it will be part of the next release :)

@hrzn hrzn merged commit 0d1a208 into unit8co:master Apr 23, 2022
@zgana zgana deleted the fix/matplotlib-config-optional branch April 24, 2022 02:36
@hrzn hrzn mentioned this pull request Dec 6, 2022
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.

[BUG] Darts matplotlib style settings should be optional
3 participants