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

Add defaults to arguments #11

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add defaults to arguments #11

wants to merge 10 commits into from

Conversation

ljwolf
Copy link
Member

@ljwolf ljwolf commented Apr 9, 2020

This adds a few enhancements relating to default arguments & alternative position/size options for loc. It also moves the examples folder to notebooks, to keep in line with other PySAL projects.

defaults

  • add a default for the breaks that is 10 evenly-spaced percentiles
  • add a default for the pal that is 'viridis'
  • add a default for the figure/axis that defaults to the current axis.

position/size options

interpret a loc = (x,y,w,h) tuple as the first two coordinates being the anchor (left bottom corner of the new axis) and the second two being the width & height of the new axis, relative to the existing axis. So, (0,0,.5,.25) would nominally start in the lefthand corner, stretch to the middle of the plot, and grow to the bottom half.

We could make that better for UX by providing a separate width ratio thing, but I think since position & width/height affect each other depending on the later matplotlib spacing calls (e.g. f.tight_layout() can really squeeze/realign the subaxis), I liked keeping this tuple as a two or four.

@ljwolf ljwolf requested review from slumnitz and darribas April 9, 2020 16:38
@ljwolf
Copy link
Member Author

ljwolf commented Apr 9, 2020

With this functionality, we get something where:

geodataframe.plot('variate')
legendgram(geodataframe.variate)

yields something that should look OK 😄

Copy link
Member

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

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

I approve, FWIW.

Copy link
Member

@darribas darribas left a comment

Choose a reason for hiding this comment

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

This looks great to me! Lots of useful bits!

Copy link
Member

@slumnitz slumnitz left a comment

Choose a reason for hiding this comment

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

Looks great, I was a little confused about the defaults here. Maybe it's worth considering?

f = plt.gcf()
if ax is None:
ax = plt.gca()
if pal is None:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am missing something here, but why do you use None and an if clause for a default? You could just directly assign 'viridis' to the parameter in the api. This way users also see how the input out to look like?

Copy link
Member

Choose a reason for hiding this comment

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

Just curious maybe I can learn something :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! I had intended to somehow detect the current matplotlib palette and use that if pal=None. But, I couldn't quite figure out how to do that. So, I deleted the exploration in this if branch & simply used pal = 'viridis'.

Ideally, you'd use pal=None and if pal is None, somehow check what colormap the user is using currently. I don't quite know how to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, yeah that sounds great!
do you think something like
'''
fig, ax = plt.subplots()
im = ax.imshow(a, cmap="viridis")

cm = im.get_cmap()
print(cm.name)
"""

through

'Matplotlib.cm.get_cmap()' could work?

Happy to leave this as is and deal with it later. Maybe add a note, so others can keep finding solutions? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, well that's simole... how I missed it idk.

No reason to merge an incomplete if that works! I'll take a look tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

@ljwolf Is this still under consideration? I put together a small gist demonstrating a helper function built on top of @slumnitz's idea. Basically, it uses the most recent colormap of an image, if the axis has any associated images. Otherwise, it defaults to viridis.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, totally! push on top of this so that it updates!

legendgram/legendgram.py Outdated Show resolved Hide resolved
ljwolf and others added 2 commits November 2, 2023 15:51
Co-authored-by: James Gaboardi <jgaboardi@gmail.com>
@jGaboardi
Copy link
Member

jGaboardi commented Nov 2, 2023

Done. No test though... 😶‍🌫️

EDIT

@ljwolf :

  • I updated the testing suite and can confirm that all pass locally (Python 3.12)
  • Found bug in the previous gist and corrected
  • Now testing for all scenarios in util._get_cmap() and confirm plotting is as expected locally
  • Coverage at 94%
  • Converted tests to pytest
  • Removed pysal.contrib.viz.mapping (xref testing for legendgrams? #7)

@jGaboardi
Copy link
Member

@ljwolf gentle ping to see if there is still interest in getting this merged.

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.

4 participants