-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Conversation
…oved deepcopy operation which is already happening in calling utils.empty_dict()
…lling utils.empty_dict()
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this 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?
figanos/matplotlib/plot.py
Outdated
levels=levels, | ||
divergent=divergent, | ||
) | ||
|
||
# matplotlib.pyplot.scatter treats "edgecolor" and "edgecolors" as aliases so we accept "edgecolor" and convert it |
There was a problem hiding this comment.
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.
figanos/matplotlib/plot.py
Outdated
"cmap": cmap, | ||
"norm": norm, | ||
"transform": transform, | ||
"zorder": 8, | ||
"marker": "o", | ||
} | plot_kw_pop | ||
"edgecolors": "none", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
if extend: | ||
fax.set_extent(extend) | ||
if extent: | ||
fax.set_extent(extent) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isinstance(plot_kw[k]["hatches"], list): | |
hc = plot_kw[k]["hatches"][0] | |
else: | |
hv = plot_kw[k]["hatches"] | |
figanos/matplotlib/plot.py
Outdated
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hatch=plot_kw[k]["hatches"][0], fill=False, label=k | |
hatch=hc, fill=False, label=k |
The Coveralls checks have been adjusted. These shouldn't fail unless coverage is dramatically impacted. |
There was a problem hiding this 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!
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 ... |
# Conflicts: # CHANGELOG.rst # src/figanos/matplotlib/plot.py
Pull Request Checklist:
number
) and pull request (:pull:number
) has been added.What kind of change does this PR introduce?
Some fixes and simplification of code:
hatchmap
.scattermap
can use’edgecolor’
and’egdecolors’
interchangeably like `matplotlib.pyplot.scatter’deepcopy
operation inhatchmap
andscattermap
which is already called infiganos.matplotlib.utils.empyt_dict(). Consequently, helper variables are renamed (
plot_kw_pop->
plot_kwin
scattermap;
dc->
plot_kwin
hatchmap`.extend
toextent
Does this PR introduce a breaking change?
No
Other information: