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

Matplotlylib fixes #3143

Merged
merged 128 commits into from
Jun 4, 2021
Merged

Matplotlylib fixes #3143

merged 128 commits into from
Jun 4, 2021

Conversation

fdion
Copy link
Contributor

@fdion fdion commented Apr 9, 2021

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

This PR fixes compatibility of matplotlylib with matplotib 3.3 and above and specifically tested with the latest release (3.4.1). Before fixing the compatibility, I fixed any matplotlylib tests that were not passing. Then I fixed the code.

Specifically, this addresses changes to matplotlib:

  • axis._gridOnMajor moved
  • spine.is_frame_like() deprecated
  • FuncFormatter instead of FixedFormatter
  • changes in dash patterns
  • number of points for bounding boxes

It also renders the shapes for the matplotib legend (previously, only the text would display in a legend). This is mainly done through the method draw_legend_shapes which is called by draw_marked_line and handles lines, markers and lines+markers. It doesn't handle the legend bounding box.

Tested with matplotlib 3.2,2 (last known working with matplotlylib, with warnings) and 3.4.1 (current), from notebooks with various charts / markers. Also tested with hotelling package with `matplotlib 3.4.1.
newplot(1)

Pascalco and others added 30 commits September 11, 2020 21:38
Later we can use this to configure base64 encoding
@@ -312,10 +312,84 @@ def draw_bar(self, coll):
"assuming data redundancy, not plotting."
)

def draw_legend_shapes(self, mode, shape, **props):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand what this function does... could you please explain or provide an example pair of matplotlib/plotly figures that uses this system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this function which is called further down on line 583 when coordinates are "axes" (plotly only handles "data"), the plots legends have no markers or lines, just text. This is why we get the warning: "Bummer! Plotly can currently only draw Line2D objects from matplotlib that are in 'data' coordinates!"

See example notebook here:

https://colab.research.google.com/drive/1-MNmqy_DeDxw8yPX0U7M9E8AEMtfIobP?usp=sharing

Matplotlib plot:
index

Plotly version:
newplot(2)

When coordinates == axes I call my new function that draws the shapes. go.layout.Shape cant handle circles, so I handle this special case, handle lines, and everything else i pass to go.layout.shape.

Result:
newplot(3)

It still is left with one warning: "I found a path object that I don't think is part of a bar chart. Ignoring."

This is due to the bounding box that in matplotlib is (now) a fancy box with curved corners. This would have to be addressed in a future PR.

@nicolaskruchten
Copy link
Contributor

Thanks for this PR, and apologies for the long time before reviewing it... I've been focused on the changes to Plotly.js for the upcoming 2.0 release, but now that that's mostly done I'm looking at merging a bunch of Python PRs :)

If you could please rebase this onto master or merge master into it, I'd appreciate it, and if you could answer my question about your second commit, I should be able to get this merged shortly!

@fdion
Copy link
Contributor Author

fdion commented Jun 2, 2021

@nicolaskruchten Merged master and documented the reason for the draw_legend_shapes function

@nicolaskruchten
Copy link
Contributor

Thanks very much for the fixes :)

@nicolaskruchten nicolaskruchten merged commit a121275 into plotly:master Jun 4, 2021
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.

None yet