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

Wrap grd2cpt #803

Merged
merged 76 commits into from
Feb 12, 2021
Merged

Conversation

willschlitzer
Copy link
Contributor

Wraps the GMT function grd2cpt. Code and documentation borrowed heavily from grdcut and makecpt.

@willschlitzer willschlitzer added the feature Brand new feature label Jan 23, 2021
@willschlitzer willschlitzer added this to the 0.3.0 milestone Jan 23, 2021
@willschlitzer
Copy link
Contributor Author

Screenshot from 2021-01-23 16-17-57
For reasons I'm not sure, I'm getting a GMTImageComparisonFailure: images not close error when I run test_grd2cpt_grdimage. I'm confused because the code for both fig_ref and fig_test are identical (as I copy and pasted to make sure I didn't have a typo). When I ran the same code in a Jupyter notebook, I also had images that looked very different. Any idea what is causing this inconsistency?

@seisman
Copy link
Member

seisman commented Jan 23, 2021

I haven't checked it carefully, but it may be related to #372

Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

I'm OK with the wrapper and the aliases, but most of the tests don't make sense to me.

For example, in the following test, if cmap="rainbow" doesn't work as expected (although unlikely as the wrapper is very simple), it would affect both the reference and test images, thus the tests always pass.

@check_figures_equal()
def test_grd2cpt_set_cpt(grid):
    """
    Test function grd2cpt to create a CPT based off a grid input and a set CPT.
    """
    fig_ref, fig_test = Figure(), Figure()
    # Use single-character arguments for the reference image
    fig_ref.basemap(B="a", J="W0/15c", R="d")
    grd2cpt(grid=grid, cmap="rainbow")
    fig_ref.colorbar(B="a2000")
    fig_test.basemap(frame="a", projection="W0/15c", region="d")
    grd2cpt(grid=grid, cmap="rainbow")
    fig_test.colorbar(frame="a2000")
    return fig_ref, fig_test

pygmt/src/grd2cpt.py Outdated Show resolved Hide resolved
@willschlitzer
Copy link
Contributor Author

I'm fine with deleting tests. I was mostly copying the format of test_makecpt.py, since that seems like the most similar module.

@seisman
Copy link
Member

seisman commented Feb 10, 2021

I'm fine with deleting tests. I was mostly copying the format of test_makecpt.py, since that seems like the most similar module.

Some of the makecpt tests make sense because they're using @pytest.mark.mpl_image_compare. I think we need to think more about these tests.

@weiji14 As you wrote the makecpt wrapper, I'll let you decide what to do.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Just a few minor comments, getting close!

pygmt/src/grd2cpt.py Outdated Show resolved Hide resolved
pygmt/src/grd2cpt.py Outdated Show resolved Hide resolved
pygmt/src/grd2cpt.py Outdated Show resolved Hide resolved
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Great! Thanks for your patience @willschlitzer, and also thank you @seisman for the comprehensive review!

@willschlitzer willschlitzer merged commit 2efcbba into GenericMappingTools:master Feb 12, 2021
@willschlitzer willschlitzer deleted the wrap-grd2cpt branch February 12, 2021 10:43
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
*Wrap grd2cpt function
*Add tests for grd2cpt function
*Modify docstring in makecpt function

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants