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

Hatchmap and scattermap fixes #195

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

Conversation

vindelico
Copy link
Contributor

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • (If applicable) Documentation has been added / updated (for bug fixes / features).
  • (If applicable) Tests have been added.
  • CHANGES.rst has been updated (with summary of main changes).
    • Link to issue (:issue:number) and pull request (:pull:number) has been added.

What kind of change does this PR introduce?

Some fixes and simplification of code:

  • Option ‘no legend’ in hatchmap.
  • Ensure ‘hatches’ are passed as a list.
  • scattermap can use ’edgecolor’ and ’egdecolors’ interchangeably like `matplotlib.pyplot.scatter’
  • removed deepcopy operation in hatchmap and scattermap which is already called in figanos.matplotlib.utils.empyt_dict(). Consequently, helper variables are renamed (plot_kw_pop->plot_kwinscattermap; dc->plot_kwinhatchmap`.
  • Correct extend to extent

Does this PR introduce a breaking change?

No

Other information:

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@sarahclaude sarahclaude left a comment

Choose a reason for hiding this comment

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

Great to have the legend bool working and hatches density with a list! Thanks Marco!

You removed plot_kw_pop, does it mean the plot_kw is not modified inside the loop to create your figures?

levels=levels,
divergent=divergent,
)

# matplotlib.pyplot.scatter treats "edgecolor" and "edgecolors" as aliases so we accept "edgecolor" and convert it
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't pop edgecolor and replace it to edgecolors since both of them work and if matplotlib ever made change to one we would have to fix it after. I also think edgecolors is more used to pass many colors as a list.

"cmap": cmap,
"norm": norm,
"transform": transform,
"zorder": 8,
"marker": "o",
} | plot_kw_pop
"edgecolors": "none",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would remove this line based on previous comment

@@ -180,7 +180,7 @@
"im = fg.hatchmap({'sup_305k': sup_305k, 'inf_300k': inf_300k},\n",
" plot_kw={\n",
" 'sup_305k': {\n",
" 'hatches': '*',\n",
" 'hatches': ['////'], # hatches must be passed as a list\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use can instead of must

figanos/matplotlib/plot.py Outdated Show resolved Hide resolved
if extend:
fax.set_extent(extend)
if extent:
fax.set_extent(extent)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if isinstance(plot_kw[k]["hatches"], list):
hc = plot_kw[k]["hatches"][0]
else:
hv = plot_kw[k]["hatches"]


pat_leg.append(
matplotlib.patches.Patch(
hatch=plot_kw[k]["hatches"], fill=False, label=k
hatch=plot_kw[k]["hatches"][0], fill=False, label=k
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
hatch=plot_kw[k]["hatches"][0], fill=False, label=k
hatch=hc, fill=False, label=k

@Zeitsperre
Copy link
Contributor

The Coveralls checks have been adjusted. These shouldn't fail unless coverage is dramatically impacted.

Copy link
Contributor

@juliettelavoie juliettelavoie left a comment

Choose a reason for hiding this comment

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

after fixing comments from sarahclaude, this looks good to me!

@juliettelavoie
Copy link
Contributor

The Coveralls checks have been adjusted. These shouldn't fail unless coverage is dramatically impacted.

Est-ce que avoir coverall c'est pertinent si on n'a pas de tests? comme c'est des figures, c'est difficile de tester ...

@github-actions github-actions bot added the docs label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants