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

fix css style #1389

Merged
merged 3 commits into from
Sep 21, 2020
Merged

fix css style #1389

merged 3 commits into from
Sep 21, 2020

Conversation

ahartikainen
Copy link
Contributor

@ahartikainen ahartikainen commented Sep 21, 2020

Description

Change access to css file.

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@codecov
Copy link

codecov bot commented Sep 21, 2020

Codecov Report

Merging #1389 into master will decrease coverage by 0.00%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1389      +/-   ##
==========================================
- Coverage   91.73%   91.72%   -0.01%     
==========================================
  Files         105      105              
  Lines       10898    10901       +3     
==========================================
+ Hits         9997     9999       +2     
- Misses        901      902       +1     
Impacted Files Coverage Δ
arviz/stats/density_utils.py 64.57% <33.33%> (-0.09%) ⬇️
arviz/plots/backends/bokeh/essplot.py 98.50% <100.00%> (ø)
arviz/plots/backends/bokeh/mcseplot.py 98.57% <100.00%> (ø)
arviz/plots/backends/bokeh/rankplot.py 93.44% <100.00%> (ø)
arviz/plots/backends/bokeh/violinplot.py 93.75% <100.00%> (ø)
arviz/plots/backends/matplotlib/essplot.py 98.91% <100.00%> (ø)
arviz/plots/backends/matplotlib/mcseplot.py 98.83% <100.00%> (ø)
arviz/plots/backends/matplotlib/rankplot.py 96.00% <100.00%> (ø)
arviz/plots/backends/matplotlib/violinplot.py 92.64% <100.00%> (ø)
arviz/plots/parallelplot.py 97.56% <100.00%> (ø)
... and 1 more

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 e355c1b...de9cb1a. Read the comment docs.

Copy link
Contributor

@aloctavodia aloctavodia left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -696,5 +696,6 @@ class HtmlTemplate:
</div>
</li>
"""
_, css_style = xr.core.formatting_html._load_static_files() # pylint: disable=protected-access
Copy link
Member

Choose a reason for hiding this comment

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

I am fine with enforcing the lazy load but we then have to update requirements to xarray 0.16.1 too, otherwise we need and if/else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, what should we do? Have our own css file, or copy the lazy load function?

Copy link
Member

Choose a reason for hiding this comment

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

I gravitate towards requiring xarray 0.16.1 so this code works without problem or having our lazy load function that calls xarray's one if possible and gets css style otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's update xarray to 0.16.1

Copy link

Choose a reason for hiding this comment

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

Just to warn you, this line is definitely using an internal xarray function that could go away or be disabled in the future. That's why you had to add the lint comment to disable protected-access :).

If you are OK copying the CSS file into arViz, that would be my preference for the safest/simplest way to do this (without any chance of things breaking in the future).

Otherwise, if for some reason you need to keep it in sync with xarray's version, we could definitely consider adding a public API option for getting this information from xarray. In that case, please open an issue in xarray's issue tracker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are using internal function. Until we figure out a good way to solve this, I think we are ok with this solution.

xarray should not (and probably are not) worry changing their internal functionality.

@ddesroches
Copy link

We ran into this issue because our project depends on Pymc3, which relies on Arviz. We are going to work around it by locking the version of XArray to before the breaking change. Your solution to rely on yet another private entry point to Xarray is a recipe for another breakage for yours and all dependent packages. Suggest if you need a coupling to XArray that you PR a new public interface to that package.

@ahartikainen
Copy link
Contributor Author

Ok, I understand. We need a better fix.

@pundivived-attention
Copy link

So how do we fix it ?

@ahartikainen
Copy link
Contributor Author

This has been fixed in #1392

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.

6 participants