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

Updates to recipe_seaborn.yml and seaborn diagnostic #3808

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aytacpacal
Copy link

Description

  • Closes #issue_number
  • Link to documentation:

Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

New or updated recipe/diagnostic


To help with the number of pull requests:

- Modified the seaborn_diag.py for jointplot
- Modified the recipe_seaborn.yml to plot the figures
- Added Pacal, Aytac to config-references.yml
@aytacpacal aytacpacal self-assigned this Nov 7, 2024
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this 👍 I have a couple of comments on the code.

Also, I wonder if it makes sense to put this into the recipes/cloud directory as some kind of recipe_clouds_scatterplot? @axel-lauer, what do you think?

Comment on lines 151 to 153
if plot_func_str in ['jointplot']:
data_frame = data_frame.reset_index()

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried running the diagnostic with the option reset_index: true?

@@ -171,7 +174,10 @@ def _create_plot(
getattr(plot_obj, func_name)(func_args)
if cfg['suptitle'] is not None:
logger.debug("Setting `suptitle='%s'`", cfg['suptitle'])
plt.suptitle(cfg['suptitle'], y=1.05)
if plot_func_str in ['jointplot']:
plt.suptitle(cfg['suptitle'].format(data_frame["shape_id"].unique()[0]), y=1.05)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause the diagnostic to fail if no shape_id is present. Is this change necessary because you created a multi-panel plot (as far as I understand, this is not possible with jointplot)? If you need such a change, it would be better to hardcode the format string into the recipe Like Plot of {shape_id} and then let the diagnostic fill this. This will be much more general.

@@ -443,6 +449,11 @@ def _set_legend_title(plot_obj, legend_title: str) -> None:
legend = plot_obj.get_legend()
elif hasattr(plot_obj, 'legend'): # FacetGrid, PairGrid
legend = plot_obj.legend
elif isinstance(plot_obj, sns.axisgrid.JointGrid): # JointGrid workaround
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
elif isinstance(plot_obj, sns.axisgrid.JointGrid): # JointGrid workaround
elif isinstance(plot_obj, sns.axisgrid.JointGrid): # JointGrid

If JointGrid also need a special treatment I wouldn't call this "workaround" 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants