Skip to content

Update test_coast_aliases #769

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

Merged

Conversation

willschlitzer
Copy link
Contributor

The current test_coast_aliases() in test_coast.py does not include all of the current aliases in PyGMT v0.2.1; I updated the function to include all of the aliases and changed the test format to use @check_figures_equal.

@seisman seisman added the maintenance Boring but important stuff for the core devs label Dec 28, 2020
@seisman
Copy link
Member

seisman commented Dec 29, 2020

Following #771 (comment), I think we don't want the changes in this PR. Am I right?

@willschlitzer
Copy link
Contributor Author

@seisman Sorry if my poor timing of submitting this pull request and #771 at the same time made this confusing. In my opinion, I think the changes in this PR are valid. I'll admit that I'm not too experienced with writing tests, but I think one of the minimum standards for PyGMT tests should be including a test that makes sure all of the aliases are successfully passed to the GMT API. My thought process here is this ensures that any changes in the GMT argument names, or any accidental changes to the PyGMT aliases, are caught. The test already existed, but my assumption is that Figure.coast has expanded to include more arguments since the test was written, so I wanted to make sure that tests all of the aliases/parameters that Figure.coast accepts.

S="skyblue",
D="i",
A=1000,
L="jMM+c1+w1000k+f+l",
Copy link
Member

Choose a reason for hiding this comment

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

MM is not a valid anchor. Do you mean CM?

Copy link
Contributor Author

@willschlitzer willschlitzer Jan 2, 2021

Choose a reason for hiding this comment

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

@seisman I did mean CM, and changed it in my most recent commit. I don't know enough about how GMT handles a different input, but it looks like MM is a valid anchor; both argument variations look the same on the figure. I ran a @check_figures_equal test for the two and it passed.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that GMT may default to "CM" if the given anchor can not be recognized.

@seisman seisman added this to the 0.3.0 milestone Jan 1, 2021
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.

The test looks OK, but it's a bad idea to use -Vq which disables warnings and errors.

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.

@willschlitzer The test looks good to me.

As you're now a member of the python-maintainers team, you should be able to merge the PR when the CI tests finish. Please try it.

It's OK if one or two Windows CI tests fail. That's a known issue (#758) and we don't have a solution yet .

@willschlitzer
Copy link
Contributor Author

@seisman sounds good!

@willschlitzer willschlitzer merged commit 478cc7a into GenericMappingTools:master Jan 2, 2021
@willschlitzer willschlitzer deleted the coast-alias-test branch January 2, 2021 10:53
@seisman
Copy link
Member

seisman commented Jan 2, 2021

@willschlitzer It's problematic to use U=True here, as it creates a timestamp and the timestep can be different for the reference and test images, which can cause failure.

Edit: Fixed at #785.

seisman added a commit that referenced this pull request Jan 2, 2021
@weiji14 weiji14 added the skip-changelog Skip adding Pull Request to changelog label Feb 6, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
* Update test_coast_aliases() for all included aliases in v0.2.1 and change test type to use @check_figures_equal

* Updating map scale anchor in alias test of test_coast.py

* Update pygmt/tests/test_coast.py

Co-authored-by: Dongdong Tian <[email protected]>

* Update pygmt/tests/test_coast.py

Co-authored-by: Dongdong Tian <[email protected]>

Co-authored-by: Dongdong Tian <[email protected]>
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants