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

Solve matplotlib style problem that was making labels non-legible #402

Merged
merged 6 commits into from
May 11, 2020

Conversation

rlopezcoto
Copy link
Contributor

Solution to work out the problem with the matplotlib style/backend that was described in #401. While this is worked out, these lines should force the matplotlib style to the 'default' one, that was working before for all the plotting.

moralejo
moralejo previously approved these changes May 11, 2020
@vuillaut
Copy link
Member

Hi

  • I just shipped ctaplot 0.5.2 fixing the styling issue - one now can actively set ctaplot style with ctaplot.set_style() if desired
  • We can still force default style in lstchain to avoid problems in the future.

@maxnoe
Copy link
Member

maxnoe commented May 11, 2020

Both these solution are not ideal.

matplotlib supports context manager for setting styles. Do not set them globally! And don't override user defined styles by setting it back to defaults.

This breaks any matplotlib mechanism to set styles outside of code (environment variables, local matplotlibrc, global matplotlibrc).

moralejo
moralejo previously approved these changes May 11, 2020
Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

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

This breaks any mechanism to set matplotlib options outside of code.

Don't mess with these things, please just fix the underlying issue.

@vuillaut
Copy link
Member

This breaks any mechanism to set matplotlib options outside of code.

Don't mess with these things, please just fix the underlying issue.

Well, as said, if the problem was indeed ctaplot, I think it is fixed.

@rlopezcoto
Copy link
Contributor Author

Agreed that this may not be ideal, but at least we need a temporary solution for all the datacheck/calibration plots to be displayed.
We can discuss a better, more general solution to remove those lines in the future, but we really need to move on with the release right now, and without this fix, the next set of reprocessed data will have very bad style, not legible plots...

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #402 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #402   +/-   ##
=======================================
  Coverage   41.44%   41.44%           
=======================================
  Files          76       76           
  Lines        6143     6143           
=======================================
  Hits         2546     2546           
  Misses       3597     3597           

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 5e24dd6...129739f. Read the comment docs.

@rlopezcoto
Copy link
Contributor Author

ok, I tested ctaplot=0.5.2 and the style is again ok, so I removed the forcing of the matplotlib "default" style.
Still, we may want to think on a way of avoiding these problems in the future...

@rlopezcoto rlopezcoto changed the title Add matplotlib style for the full repo Solve matplotlib style problem that was making labels non-legible May 11, 2020
@vuillaut
Copy link
Member

ok, I tested ctaplot=0.5.2 and the style is again ok, so I removed the forcing of the matplotlib "default" style.
Still, we may want to think on a way of avoiding these problems in the future...

Glad this is fixed and sorry for the mess.

@rlopezcoto rlopezcoto merged commit 889a461 into cta-observatory:master May 11, 2020
@rlopezcoto rlopezcoto deleted the solve_backend_problem branch May 11, 2020 08:37
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.

4 participants