Fixed plot_trace legend overwriting by chain legend (Issue #2320)

imperorrp commented 6 months ago

Description

Addresses Issue #2320 , where chain legend seemed to overwrite fist subplot legend that displays coord combinations.

When the chain legend is created (.legend(handles=handles, title="chain", loc="upper right")), passing this to second subplot in the plot generated, which is the first trace plot (ax.figure.axes[1]) instead of the first subplot, which is the first density plot and already has a legend (ax.figure.axes[0]) prevents overwriting. As per @OriolAbril 's suggestion in the issue comments of #2320 .

Here are the plots now generated with this small edit, with no overwriting:

Checklist

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

📚 Documentation preview 📚: https://arviz--2334.org.readthedocs.build/en/2334/

imperorrp commented 6 months ago

Test failures seem to be from an issue with checking axes[0,0] for the chain legend- could this test be modified?

OriolAbril commented 6 months ago

Test failures seem to be from an issue with checking axes[0,0] for the chain legend- could this test be modified?

Yes, it needs to be modified. Once you have you can check the " Includes new or updated tests to cover the new feature" box

imperorrp commented 6 months ago

Test failures seem to be from an issue with checking axes[0,0] for the chain legend- could this test be modified?

Yes, it needs to be modified. Once you have you can check the " Includes new or updated tests to cover the new feature" box

Sure, will modify test as appropriate

imperorrp commented 6 months ago

If you click at the "details" link of the failing job you'll eventually get to this page: https://dev.azure.com/ArviZ/ArviZ/_build/results?buildId=6712&view=logs&jobId=52053870-44e4-57aa-b540-f1a2d094f440&j=52053870-44e4-57aa-b540-f1a2d094f440&t=b4fdbffc-b4d0-53ca-c47c-a902d6813a77.

It looks like there is a trailing whitespace somewhere. You can run black which will get rid of it and if necessary to any other formatting changes needed. With that and adding a line to the Changelog in maintenance and bug fixes section it will be ready to merge.

Sure, just did that. Seems some doc/ files got formatted too- is that alright or is black formatting not supposed to be run on that folder?

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.84%. Comparing base (339991b) to head (c7c1e73).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2334 +/- ## ======================================= Coverage 86.84% 86.84% ======================================= Files 123 123 Lines 12745 12745 ======================================= Hits 11068 11068 Misses 1677 1677 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

OriolAbril commented 6 months ago

It's ok yeah, don't worry.

OriolAbril commented 6 months ago

Let's add a bullet point to the changelog to check the last box and merge

imperorrp commented 5 months ago

Let's add a bullet point to the changelog to check the last box and merge

Sure, just did that

arviz-devs / arviz