Skip to content

Change text when GMTInvalidInput error is raised for basemap #729

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.

Sign up for GitHub

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
merged 19 commits into from
Dec 15, 2020
Merged

Change text when GMTInvalidInput error is raised for basemap #729

Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7be1d62
Change error text for basemap GMTInvalidInput
willschlitzer Dec 14, 2020
32edc8e
Remove rose from basemap GMTInvalidInput error; adding "rose" or "com…
willschlitzer Dec 14, 2020
815876b
Add basemap GMTInvalidInput exception if region is not specified.
willschlitzer Dec 14, 2020
c9831b0
Rewrite if statement for R in kwargs under basemap
willschlitzer Dec 14, 2020
5efce1c
Removing "At least" from GMTInvalidInput error text since numbers are…
willschlitzer Dec 14, 2020
ed798ad
Remove "T in kwargs" from if not statement, as "T" is not an availabl…
willschlitzer Dec 14, 2020
41e8d48
Remove reqion error raising for basemap()
willschlitzer Dec 14, 2020
6de6750
Update if statement and error text for compass or rose
willschlitzer Dec 15, 2020
bf054c2
Merge branch 'master' into baseplotting-error
willschlitzer Dec 15, 2020
d0dc60b
Run make format
willschlitzer Dec 15, 2020
5a705bc
Add test_basemap_compass() and test_basemap_rose()
willschlitzer Dec 15, 2020
8abd940
Run make format
willschlitzer Dec 15, 2020
fb5abf4
Change GMTInvalidInput error text for basemap in base_plotting.py
willschlitzer Dec 15, 2020
7de49a5
Fix tests in test_basemap.py
willschlitzer Dec 15, 2020
10b59fe
Adding comparison images that I forgot to add
willschlitzer Dec 15, 2020
375aa73
Rewrite compass and rose tests to use @check_figures_equal() and dele…
willschlitzer Dec 15, 2020
ca740d5
Update pygmt/tests/test_basemap.py
willschlitzer Dec 15, 2020
acac815
Update pygmt/tests/test_basemap.py
willschlitzer Dec 15, 2020
412e390
Merge branch 'master' into baseplotting-error
willschlitzer Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pygmt/base_plotting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1167,8 +1167,8 @@ def basemap(self, **kwargs):

"""
kwargs = self._preprocess(**kwargs)
if not ("B" in kwargs or "L" in kwargs or "T" in kwargs):
raise GMTInvalidInput("At least one of B, L, or T must be specified.")
if not ("B" in kwargs or "L" in kwargs):
raise GMTInvalidInput("Frame or map_scale must be specified.")
Copy link
Member

Choose a reason for hiding this comment

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

We actually have Tm and Td, but agree that the error message should be reworded.

pygmt/pygmt/base_plotting.py

Lines 1120 to 1121 in 2128dd4

Td="rose",
Tm="compass",

Suggested change
if not ("B" in kwargs or "L" in kwargs):
raise GMTInvalidInput("Frame or map_scale must be specified.")
if not ("B" in kwargs or "L" in kwargs or "T" in kwargs):
raise GMTInvalidInput("At least one of frame, map_scale, rose or compass must be specified.")

Not sure if we should have separate checks for "Td" and "Tm" though, or if "T" is enough? Could you stress test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weiji14 I mentioned Tm and Td in #730 as what I assume are the aliases being referred to with T. I'm not familiar with the rose and compass arguments and haven't managed to plot them on a map, but I have at least had them run without raising an error.

I passed:
fig.basemap(region=[0, 10, 0, 10], frame="af", compass="x1/2/1")

and

fig.basemap(region=[0, 10, 0, 10], frame="af", rose="x1/2/1")

Both of these ran without an error (although I couldn't get the compass or rose to actually plot), but the GMTInvalidInput was raised when I deleted frame.

Copy link
Member

Choose a reason for hiding this comment

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

See if this test case provides some inspiration (this was written quite a while ago before the compass/ross alias was done):

pygmt/pygmt/tests/test_config.py

Lines 61 to 70 in e6d45ec

def test_config_font_annot():
"""
Test that setting `FONT_ANNOT` config changes both `FONT_ANNOT_PRIMARY` and
`FONT_ANNOT_SECONDARY`.
"""
fig = Figure()
with config(FONT_ANNOT="6p,red"):
fig.basemap(region=[0, 9, 0, 9], projection="C3/3/9c", T="mjTL+w4c+d4.5")
fig.basemap(T="mjBR+w5c+d-4.5")
return fig

Basically something like fig.basemap(compass="jBR+w5c+d-4.5")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weiji14 I'll take a look at this tomorrow.

Also, is there any reason the tests use the GMT aliases instead of the PyGMT parameters when calling functions? I was to try and add tests for map_scale, compass, and rose arguments to make sure the error isn't raised when they're passed.

Copy link
Member

Choose a reason for hiding this comment

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

Part of the reason is historical. We did a big purge of short aliases in #474 on the user-facing side, but haven't got around to fixing the backend tests. That said, there are some tests which do explicitly use short GMT aliases, but the one I linked above should not be one of them.

TLDR: Use long aliases unless there's a reason to use short ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weiji14 Thanks for the info!

Regarding stress testing GMT accepting Td/Tm values, is there any easy way to use the pygmt folder from my repo in the environment instead of v0.2.1 that I have in my conda environment, or is writing good tests the better solution here? I'm not opposed to writing tests, but it's a lengthy process to have to wait for tests to run after pushing a commit.

Copy link
Member

Choose a reason for hiding this comment

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

is there any easy way to use the pygmt folder from my repo in the environment instead of v0.2.1 that I have in my conda environment

You can activate your conda environment, run cond uninstall pygmt to uninstall the v0.2.1 version, and then change into your pygmt folder, and reinstall pygmt using pip install -e .. The command installs pygmt to your environment in "editable/developing" mode. Now in the conda environment, you can use the pygmt master or any other branches.

it's a lengthy process to have to wait for tests to run after pushing a commit.

You don't have to rely on the CI tests. Instead, you can run all tests locally by running make test, or run tests in a specific file by pytest pygmt/tests/test_coast.py.

Copy link
Member

Choose a reason for hiding this comment

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

If you do make install (i.e. pip install -e), that will install PyGMT in 'editable' mode so any changes should work once you reload the PyGMT library or use importlib.reload. As for tests, you can run just one file at a time following https://github.com/GenericMappingTools/pygmt/blob/master/CONTRIBUTING.md#testing-your-code, @seisman just simplified things with #725:

pytest pygmt/tests/test_somefile.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the help with this! It looks like compass and rose do not individually satisfy the requirement and the error is still raised. Working on updating my pull request.

with Session() as lib:
lib.call_module("basemap", build_arg_string(kwargs))

Expand Down