Change text when GMTInvalidInput error is raised for basemap

[willschlitzer]

When using basemap in base_plotting.py, the function will raise an exception if frame or map scale are not passed as an argument, but the text refers to the GMT aliases for them instead of the PyGMT parameter names. I changed the error text to read "frame or map_scale" and I deleted "T" from the parameters to check for as there is no "T" alias for basemap.

[weiji14]

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?

[willschlitzer]

@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.

[weiji14]

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")

[willschlitzer]

@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.

[weiji14]

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.

[willschlitzer]

@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.

[seisman]

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.

[weiji14]

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

[willschlitzer]

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.

[seisman]

@willschlitzer You may want to read the section about testing plots, and also the test test_coast_iceland() in pygmt/tests/test_coast.py.

[seisman]

@seisman I'm assuming this in reference to me missing the last line where it said "Don't forget to commit the baseline image as well." Or am I missing something else?

No. There are two different ways to test plots (see more details in the CONTRIBUTING.md file)

1. Using @check_figures_equal() (the recommended way)
2. Using @pytest.mark.mpl_image_compare (the way you're doing)

When we started the project, we used the 2nd way, but we realized that it stores large baseline images in the repository and significantly increase the repository size. Then we switched to the 1st way, which can avoid the problem (see #451 for the historical discussions).

[willschlitzer]

@seisman I saw that @check_figures_equal() was recommended but saw that the test_basemap.py didn't use it. I'll rewrite the test.

[willschlitzer]

@seisman Is there a push for the tests using @pytest.mark.mpl_image_compare to be changed to use @check_figures_equal() if possible? Or should the existing tests be left alone for the time being?

[seisman]

We didn't discuss it yet, but IMHO we should change @pytest.mark.mpl_image_compare to @check_figures_equal() if possible. @weiji14 Any thoughts?

[weiji14]

I wouldn't recommend rewriting any test unless absolutely necessary since they're there to prevent regressions. Also 1) Time could be better spent on other things, 2) The @check_figures_equal() test is actually slower as it runs twice compared to @mpl_image_compare.

In the medium- to long- term though, e.g. with GMT themes (in GMT 6.2.0?), the test rewrite might need to happen. There's also the related issue of vendoring off @check_figures_equal at #579 instead of having it in-house.

[seisman]

e.g. with GMT themes (in GMT 6.2.0?), the test rewrite might need to happen.

Yes, the "GMT themes" feature will be merged recently and GMT 6.2.0 will probably be released in late Jan. or early Feb, then we have to make the changes/decisions.

[willschlitzer]

I'm not familiar with what happens under the hood with the GMT API or how GMT Themes will affect PyGMT plots, but I'm assuming this means that all of the reference figures will have to be remade to pass the mpl_image_compare tests? I don't mean to advocate for rewriting tests for the sake of rewriting tests, but wouldn't it be best to have the tests rewritten before the release to enable a quicker switch to using GMT 6.2?

[seisman]

I'm not familiar with what happens under the hood with the GMT API or how GMT Themes will affect PyGMT plots

Here is the issue (GenericMappingTools/gmt#2290) and PR (GenericMappingTools/gmt#3344) related to the "GMT themes" we mentioned.

I'm assuming this means that all of the reference figures will have to be remade to pass the mpl_image_compare tests?

I think so.

I don't mean to advocate for rewriting tests for the sake of rewriting tests, but wouldn't it be best to have the tests rewritten before the release to enable a quicker switch to using GMT 6.2?

As you may already notice it, we are testing PyGMT with both GMT 6.1.1 and the master branch, it means that when the "GMT themes" PR is merged, we will see that all test pass for GMT 6.1.1 but some tests pass for GMT master. We can address the failure tests at that time (before GMT 6.2 is released).

[seisman]

FYI, GMT 6.2 may be released in late Jan. or early Feb. Likely, we will release PyGMT v0.3.0 before GMT 6.2 release, so PyGMT v0.3.0 will be the last version that supports GMT 6.1. Then we will bump the minimum required GMT version to 6.2 for PyGMT v0.4.0.

[willschlitzer]

Should we then specify the GMT version in the conda environment setup code to keep future users from downloading PyGMT v0.3.0 and GMT 6.2? I'm assuming conda/pip would otherwise grab the latest released versions of both packages.

[seisman]

I think we already specify the minimum GMT version for the conda recipe:

https://github.com/conda-forge/pygmt-feedstock/blob/master/recipe/meta.yaml

and, GMT is not available for pip.

[willschlitzer]

Sorry if that was unclear. My point was that if PyGMT v0.3.0 isn't compatible with GMT 6.2, wouldn't that be a cause for concern as conda will automatically install the latest available GMT even if it isn't compatible? My thought is that the suggested install command would say conda create --name pygmt python=3.9 pip numpy pandas xarray netcdf4 packaging gmt=6.1.1 when GMT 6.2 is out but PyGMT v0.3.0 is still the latest release.

[weiji14]

Actually, PyGMT v0.3.0 will be compatible with GMT 6.2.0, just that the default theme of the plots will look different than if you were using PyGMT with GMT 6.1.1. Breaking tests isn't really a bad thing in this case, since we know the reason they broke (GMT theme change) so I don't think we need to worry about setting a hard version pin for now.

[willschlitzer]

@weiji14 Sounds good! Thanks for clarifying that!

[weiji14]

No worries, I've opened up an issue at #748 so we can clarify this somewhere in our documentation.