Wrap histogram

[willschlitzer]

Wrapping the histogram module

Preview at https://pygmt-git-wrap-histogram-gmt.vercel.app/api/generated/pygmt.Figure.histogram.html

Fixes #563

Reminders

• Run make format and make check to make sure the code follows the style guide.
• Add tests for new features or tests that would have caught the bug that you're fixing.
• Add new public functions/methods/classes to doc/api/index.rst.
• Write detailed docstrings for all functions/methods.
• If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

• /format: automatically format and lint the code
• /test-gmt-dev: run full tests on the latest GMT development version

[willschlitzer]

I'm not sure how to use the virtualfile to import a table. I'm trying:
table = [1,1,1,1,1,1,2,2,2,3,4,5,6,7,8,8,8,8,8,8]
np_table = np.array(table)
fig.histogram(table=np_table, projection="X10c/25c", T=1, G="green")

But I get TypeError: histogram() got multiple values for argument 'table'

I'm not sure how to fix this, as it seems like I'm passing the correct input in (but clearly not).Any ideas @GenericMappingTools/python ?

[seisman]

I would suggest you only focus on the plotting part in this PR, and implement the inquire (-I) feature in a separate PR (after some discussions as in #896).

[weiji14]

I would suggest you only focus on the plotting part in this PR, and implement the inquire (-I) feature in a separate PR (after some discussions as in #896).

Just on this point (and as I've mentioned to Jiayuan at #1145 (review)), I'd recommend wrapping as few aliases as you can get away with in this PR. We can open up separate PRs to wrap any complicated parameter like inquire=I, or transparency=t, and the gallery example/tutorial can be left for another day too.

To be clear, that means you just have to alias the required arguments (https://docs.generic-mapping-tools.org/dev/histogram.html#required-arguments), and the common aliases (-R, -J, -c, -p, -t, etc) to pass. Maybe add one minimal unit test to hit the code coverage target. I see you've done a few aliases already (and you can leave them in if you want), but don't feel like you have to complete all of them.

Example workflow would be to do: Minimal PR (this one) -> Gallery Example PR -> Complete documentation/aliases PR -> Full Tutorial PR

P.S. Yes, we should document this 'policy' about wrapping modules chunk by chunk somewhere, but thought I'd mention it to you first.

[willschlitzer]

I would suggest you only focus on the plotting part in this PR, and implement the inquire (-I) feature in a separate PR (after some discussions as in #896).

Just on this point (and as I've mentioned to Jiayuan at #1145 (review)), I'd recommend wrapping as few aliases as you can get away with in this PR. We can open up separate PRs to wrap any complicated parameter like inquire=I, or transparency=t, and the gallery example/tutorial can be left for another day too.

To be clear, that means you just have to alias the required arguments (https://docs.generic-mapping-tools.org/dev/histogram.html#required-arguments), and the common aliases (-R, -J, -c, -p, -t, etc) to pass. Maybe add one minimal unit test to hit the code coverage target. I see you've done a few aliases already (and you can leave them in if you want), but don't feel like you have to complete all of them.

Example workflow would be to do: Minimal PR (this one) -> Gallery Example PR -> Complete documentation/aliases PR -> Full Tutorial PR

P.S. Yes, we should document this 'policy' about wrapping modules chunk by chunk somewhere, but thought I'd mention it to you first.

I'm on board with with suggested workflow. I think it keeps the pull request more manageable by not including everything under one review,

[willschlitzer]

Sorry for the wait @willschlitzer. I'm just about ready to approve this and put the final-review-call label on. If you could please update the branch, and re-create the histogram baseline image using GMT 6.2.0rc1 (or we can do it for you if you don't want to upgrade your conda environment yet) and we'll try and get this merged in the next few days, barring any major issues crossed_fingers

@weiji14 Updated the baseline image with my new GMT 6.2.0rc1 environment (took much longer than I'm proud to admit to find out to use the current working branch of PyGMT in your conda environment; PR coming soon to add that to contributing/install guidelines).

[weiji14]

Alright, glad you managed to update to GMT 6.2.0rc1, I agree that the upgrade process could be a lot less painful (but that's for a separate issue). Anyways, time for the final review call!

[seisman]

Is the baseline image correct? I expect to see y=6 for x=8.

BTW, please change projection to a smaller dimension like X10c/5c.

[willschlitzer]

@seisman I just tried it on both GMT 6.1.1 and 6.2.0rc1, with the figure coming out the same way. Any idea what might be causing the miscount for 8?

Also, changing the projection to X3c/10c to give it a larger vertical scale; does that work?

[seisman]

It looks like an upstream bug.

[willschlitzer]

I'll try and recreate it in GMT and open an upstream issue

[seisman]

GMT automatically determines the x range from input data. In this example, xmin=1, xmax=8, but the vertical bar for x=8 is outside of the [1, 8] range, so it's not plotted.

You should be able to see the x=8 bar, if you manually set region=[0, 9, 0, 6]. I think we can do it temporarily so that this PR can be merged, and report it to upstream and wait for feedback and potential fix.

[seisman]

Also, changing the projection to X3c/10c to give it a larger vertical scale; does that work?

BTW, the large vertical scale makes the image difficult to read, why not just use "X10c/10c" for a square basemap?

[willschlitzer]

@seisman I made your suggested changes

[seisman]

The "Style Checks" fail but is unrelated to changes in this PR:

************* Module pygmt.figure
pygmt/figure.py:82:8: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)