Setting frame=None gives unexpected results

[jbusecke]

Description of the problem

I am just getting started with pygmt and ran through the first example in the docs.

I wanted to play a bit with the example and tried to deactivate the frame

Full code that generated the error
When I modify

fig.basemap(region=[-90, -70, 0, 20], projection="M15c", frame=True)

to

fig.basemap(region=[-90, -70, 0, 20], projection="M15c", frame=None)

I get this rather cryptic error

Full error message

basemap [ERROR]: Must specify at least one of -A, -B, -D, -L, -T
---------------------------------------------------------------------------
GMTCLibError                              Traceback (most recent call last)
/tmp/ipykernel_493/1135014409.py in <module>
----> 1 fig.basemap(region=[-90, -70, 0, 20], projection="M15c", frame=None)

/srv/conda/envs/notebook/lib/python3.8/site-packages/pygmt/helpers/decorators.py in new_module(*args, **kwargs)
    584                     )
    585                     warnings.warn(msg, category=SyntaxWarning, stacklevel=2)
--> 586             return module_func(*args, **kwargs)
    587 
    588         new_module.aliases = aliases

/srv/conda/envs/notebook/lib/python3.8/site-packages/pygmt/helpers/decorators.py in new_module(*args, **kwargs)
    724                         kwargs[arg] = separators[fmt].join(f"{item}" for item in value)
    725             # Execute the original function and return its output
--> 726             return module_func(*args, **kwargs)
    727 
    728         return new_module

/srv/conda/envs/notebook/lib/python3.8/site-packages/pygmt/src/basemap.py in basemap(self, **kwargs)
     82         )
     83     with Session() as lib:
---> 84         lib.call_module("basemap", build_arg_string(kwargs))

/srv/conda/envs/notebook/lib/python3.8/site-packages/pygmt/clib/session.py in call_module(self, module, args)
    498         )
    499         if status != 0:
--> 500             raise GMTCLibError(
    501                 f"Module '{module}' failed with status code {status}:\n{self._error_message}"
    502             )

GMTCLibError: Module 'basemap' failed with status code 72:
basemap [ERROR]: Must specify at least one of -A, -B, -D, -L, -T

Since None is the default for the frame kwarg I am a bit confused here. Is frame=True always required?

System information

Please paste the output of python -c "import pygmt; pygmt.show_versions()":

PyGMT information:
  version: v0.5.0
System information:
  python: 3.8.12 | packaged by conda-forge | (default, Sep 29 2021, 19:52:28)  [GCC 9.4.0]
  executable: /srv/conda/envs/notebook/bin/python
  machine: Linux-5.4.120+-x86_64-with-glibc2.10
Dependency information:
  numpy: 1.21.2
  pandas: 1.3.3
  xarray: 0.19.0
  netCDF4: 1.5.7
  packaging: 21.0
  ghostscript: 9.54.0
  gmt: 6.2.0
GMT library information:
  binary dir: /srv/conda/envs/notebook/bin
  cores: 4
  grid layout: rows
  library path: /srv/conda/envs/notebook/lib/libgmt.so
  padding: 2
  plugin dir: /srv/conda/envs/notebook/lib/gmt/plugins
  share dir: /srv/conda/envs/notebook/share/gmt
  version: 6.2.0

[jbusecke]

Ah I just saw this: At least one of the parameters frame, map_scale, rose or compass must be specified. here.

I find this signature very confusing. Is there a way to provide a reasonable default (like frame=True), so that a call to fig.basemap() does not raise an error? I might also misunderstand something here.

[weiji14]

@willschlitzer, do you want to have a think about the frame parameter here since you're working on revamping the first tutorial at #1607?

[willschlitzer]

@willschlitzer, do you want to have a think about the frame parameter here since you're working on revamping the first tutorial at #1607?

I like the idea of setting frame=True as the default for basemap, as I think that is much more likely to be plotted than map_scale, rose, or compass when plotting basemap.

Admittedly, I also think basemap should be rarely used when creating plots, as frames and titles can also be set in functions like coast, plot, grdimage, and the like. I chose not to include it on the new first_figure.py because I think it adds extra complexity when an adequate map can be created using only coast.

[seisman]

I think this issue actually contains a bug report and a feature request.

Bug report

In the current implementation, running the following code

fig.basemap(region=[-90, -70, 0, 20], projection="M15c")

raises an exception At least one of frame, map_scale, compass, rose, or panel must be specified.

Because the default value of frame is None, the following code should raise the same exception.

fig.basemap(region=[-90, -70, 0, 20], projection="M15c", frame=None)

However, as reported in this issue, the above code doesn't raise the exception. Instead, GMT reports an error basemap [ERROR]: Must specify at least one of -A, -B, -D, -L, -T, which make little sense to PyGMT users.

The problem is that when frame=None is passed, the use_alias decorator converts it to kwargs["B"]=None, so the following if-test is False and the exception is not raised:

pygmt/pygmt/src/basemap.py

Lines 79 to 82 in 4186400

	if not args_in_kwargs(args=["B", "L", "Td", "Tm", "c"], kwargs=kwargs):
	raise GMTInvalidInput(
	"At least one of frame, map_scale, compass, rose, or panel must be specified."
	)

This is a general issue for all aliased options and should be fixed.

feature request

As discussed in above comments, when none of -A, -B, -D, -L, -T is passed, it makes sense to set frame=True, rather than raising an exception.

[weiji14]

Actually, is this a side effect of #1125?

[weiji14]

This is a general issue for all aliased options and should be fixed.

I may have got a generic solution for the bug fix in #1815 by modifying the args_in_kwargs function, but needs a bit more testing and handling of edge cases.

feature request

As discussed in above comments, when none of -A, -B, -D, -L, -T is passed, it makes sense to set frame=True, rather than raising an exception.

This would need to be handled in a separate Pull Request. I suppose we could modify these lines, so that kwargs["B"] is set to True if no arguments are passed to A/B/D/L/T.

pygmt/pygmt/src/basemap.py

Lines 79 to 82 in 4186400

	if not args_in_kwargs(args=["B", "L", "Td", "Tm", "c"], kwargs=kwargs):
	raise GMTInvalidInput(
	"At least one of frame, map_scale, compass, rose, or panel must be specified."
	)

[seisman]

Actually, is this a side effect of #1125?

Yes, it's related to #1125, but it isn't a side effect of #1125. As I understand the code, the behavior of frame=None should not change before or after #1125.

I may have got a generic solution for the bug fix in #1815 by modifying the args_in_kwargs function, but needs a bit more testing and handling of edge cases.

I don't think your solution in #1815 is general, because sometimes we don't use args_in_kwagrs, instead, we simply check if "B" in kwargs or similar.

I think a more general solution is dealing with frame=None (i.e. removing kwargs["B"] if it's None) in the @use_alias decorator.

[maxrjones]

I may have got a generic solution for the bug fix in #1815 by modifying the args_in_kwargs function, but needs a bit more testing and handling of edge cases.

I don't think your solution in #1815 is general, because sometimes we don't use args_in_kwagrs, instead, we simply check if "B" in kwargs or similar.

I think a more general solution is dealing with frame=None (i.e. removing kwargs["B"] if it's None) in the @use_alias decorator.

I think we should refactor those cases that use logic like if "B" in kwargs" to use get rather than adding more functionality to the use_alias decorator.

[weiji14]

I may have got a generic solution for the bug fix in #1815 by modifying the args_in_kwargs function, but needs a bit more testing and handling of edge cases.

I don't think your solution in #1815 is general, because sometimes we don't use args_in_kwagrs, instead, we simply check if "B" in kwargs or similar.
I think a more general solution is dealing with frame=None (i.e. removing kwargs["B"] if it's None) in the @use_alias decorator.

I think we should refactor those cases that use logic like if "B" in kwargs" to use get rather than adding more functionality to the use_alias decorator.

Agree with using .get, but we'll need to make sure it treats 0 as a number and not as a boolean False (see #1815 (comment)).

[maxrjones]

Agree with using .get, but we'll need to make sure it treats 0 as a number and not as a boolean False (see #1815 (comment)).

This isn't a solution for #1815 (comment), but elsewhere if kwargs.get(key) is (not) None: should not be affected by this edge-case.

[seisman]

I may have got a generic solution for the bug fix in #1815 by modifying the args_in_kwargs function, but needs a bit more testing and handling of edge cases.

I don't think your solution in #1815 is general, because sometimes we don't use args_in_kwagrs, instead, we simply check if "B" in kwargs or similar.
I think a more general solution is dealing with frame=None (i.e. removing kwargs["B"] if it's None) in the @use_alias decorator.

I think we should refactor those cases that use logic like if "B" in kwargs" to use get rather than adding more functionality to the use_alias decorator.

I'm OK with the .get method, but we must be very careful not to use if "B" in kwargs when writing new codes.

[seisman]

@weiji14 Do you plan to do the .get refactor in #1815? Or someone does it in a separate PR?

[weiji14]

Yep, I can refactor #1815 to put the logic in the @use_alias decorator. Just give me a few hours...

[seisman]

Yep, I can refactor #1815 to put the logic in the @use_alias decorator. Just give me a few hours...

Wait... I thought in #1665 (comment) @meghanrjones meant to not change the @use_alias decorator.

[weiji14]

Yep, I can refactor #1815 to put the logic in the @use_alias decorator. Just give me a few hours...

Wait... I thought in #1665 (comment) @meghanrjones meant to not change the @use_alias decorator.

Oh right, so we're using .get() on a case by case basis then? I can go with that.

[maxrjones]

Oh right, so we're using .get() on a case by case basis then? I can go with that.

Yes, I prefer case-by-base because I think the @use_alias decorator already does too much (i.e., is too complicated and not very readable) and that we should look towards getting rid of it once all aliases are wrapped so that the code in the modules is more readable (e.g., kwargs.get("frame") versus kwargs.get("B")) (xref #262, which originally referenced getting rid of @use_alias).

Fun opportunity to use walrus operators now that we dropped 3.7, I am a bit jealous 😄

[weiji14]

Actually, I might do 2 PRs

1. Refactor args_in_kwargs in None shall not pass args_in_kwargs #1815 which should fix the problem in basemap, coast, grdgradient, etc that use args_in_kwargs.
2. Replace if "I" in kwargs with kwargs.get("I") is not None to fix Figure.grdimage does not respect shading=None #1852 and other functions that do that.

[seisman]

As discussed in above comments, when none of -A, -B, -D, -L, -T is passed, it makes sense to set frame=True, rather than raising an exception.

I'm reopening the issue to track the above feature request.