-
Notifications
You must be signed in to change notification settings - Fork 226
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
Allow passing arguments containing spaces into pygmt functions #1487
Changes from 19 commits
b5ad405
14648c2
924f439
707745f
03e4308
095449a
d81b80b
dd849cb
c29e632
83c8c3b
2ba8420
b58af8b
3ec7727
f19c41b
7a518a1
9774d5e
ff40d27
36fdec5
6c399ba
4770396
801ba01
ecb580e
a526d45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,7 +120,7 @@ def dummy_context(arg): | |
|
||
|
||
def build_arg_string(kwargs): | ||
""" | ||
r""" | ||
Transform keyword arguments into a GMT argument string. | ||
|
||
Make sure all arguments have been previously converted to a string | ||
|
@@ -131,6 +131,11 @@ def build_arg_string(kwargs): | |
same command line argument. For example, the kwargs entry ``'B': ['xa', | ||
'yaf']`` will be converted to ``-Bxa -Byaf`` in the argument string. | ||
|
||
Note that spaces ` ` in arguments are converted to the equivalent octal | ||
code `\040`, except in the case of -J (projection) arguments where PROJ4 | ||
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
strings (e.g. "+proj=longlat +datum=WGS84") will have their spaces removed. | ||
See https://github.com/GenericMappingTools/pygmt/pull/1487 for more info. | ||
|
||
Parameters | ||
---------- | ||
kwargs : dict | ||
|
@@ -151,7 +156,7 @@ def build_arg_string(kwargs): | |
... A=True, | ||
... B=False, | ||
... E=200, | ||
... J="X4c", | ||
... J="+proj=longlat +datum=WGS84", | ||
... P="", | ||
... R="1/2/3/4", | ||
... X=None, | ||
|
@@ -160,7 +165,7 @@ def build_arg_string(kwargs): | |
... ) | ||
... ) | ||
... ) | ||
-A -E200 -JX4c -P -R1/2/3/4 -Z0 | ||
-A -E200 -J+proj=longlat+datum=WGS84 -P -R1/2/3/4 -Z0 | ||
>>> print( | ||
... build_arg_string( | ||
... dict( | ||
|
@@ -176,6 +181,16 @@ def build_arg_string(kwargs): | |
Traceback (most recent call last): | ||
... | ||
pygmt.exceptions.GMTInvalidInput: Unrecognized parameter 'watre'. | ||
>>> print( | ||
... build_arg_string( | ||
... dict( | ||
... B=["af", "WSne+tBlank Space"], | ||
... F='+t"Empty Spaces"', | ||
... l="'Void Space'", | ||
... ), | ||
... ) | ||
... ) | ||
-BWSne+tBlank\040Space -Baf -F+t"Empty\040\040Spaces" -l'Void\040Space' | ||
""" | ||
gmt_args = [] | ||
|
||
|
@@ -185,11 +200,19 @@ def build_arg_string(kwargs): | |
if kwargs[key] is None or kwargs[key] is False: | ||
pass # Exclude arguments that are None and False | ||
elif is_nonstr_iter(kwargs[key]): | ||
gmt_args.extend(f"-{key}{value}" for value in kwargs[key]) | ||
for value in kwargs[key]: | ||
_value = str(value).replace(" ", r"\040") | ||
gmt_args.append(rf"-{key}{_value}") | ||
elif kwargs[key] is True: | ||
gmt_args.append(f"-{key}") | ||
else: | ||
gmt_args.append(f"-{key}{kwargs[key]}") | ||
if key != "J": # non-projection parameters | ||
_value = str(kwargs[key]).replace(" ", r"\040") | ||
else: | ||
# special handling if key == "J" (projection) | ||
# remove any spaces in PROJ4 string | ||
_value = str(kwargs[key]).replace(" ", "") | ||
gmt_args.append(rf"-{key}{_value}") | ||
Comment on lines
+209
to
+215
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know almost nothing about PROJ4, but does a PROJ4 string always contains
The motivation is, since PROJ4 is not commonly used, it seems a waste of time to do call the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if all PROJ-strings have either Also unsure if 2 extra if-checks saves time, considering that either way, a |
||
return " ".join(sorted(gmt_args)) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,7 +55,7 @@ def __init__(self, **kwargs): | |
self.old_defaults[key] = lib.get_default(key) | ||
|
||
# call gmt set to change GMT defaults | ||
arg_str = " ".join([f"{key}={value}" for key, value in kwargs.items()]) | ||
arg_str = " ".join([f'{key}="{value}"' for key, value in kwargs.items()]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I've fixed the |
||
with Session() as lib: | ||
lib.call_module("set", arg_str) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
outs: | ||
- md5: e6984efed2a94673754cc7f1f1d74832 | ||
size: 9069 | ||
path: test_basemap_utm_projection.png |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
outs: | ||
- md5: 3619720cdfcd857cbdbb49ed7fe6e930 | ||
size: 1392 | ||
path: test_config_format_date_map.png |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
outs: | ||
- md5: 8e1c47b1cf6001dad3b3c0875af4562e | ||
size: 150390 | ||
- md5: ce2d5cd1415b7c7bbeea5bf6ff39c480 | ||
size: 150288 | ||
path: test_rose_no_sectors.png |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,7 +152,7 @@ def test_rose_no_sectors(data_fractures_compilation): | |
region=[0, 500, 0, 360], | ||
diameter="10c", | ||
labels="180/0/90/270", | ||
frame=["xg100", "yg45", "+t'Windrose diagram'"], | ||
frame=["xg100", "yg45", "+tWindrose diagram"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it does still work, but the single quotes would be printed. I think it was a typo from whoever wrote that test. |
||
pen="1.5p,red3", | ||
transparency=40, | ||
scale=0.5, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a check before this that
prefix
is provided so that the user doesn't get a KeyError here? Or you could raise GMTInvalidInput in an except statement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Looking at the docs at https://docs.generic-mapping-tools.org/6.3/psconvert.html, it reads like
prefix
/-F
is optional, but when I tried runningpsconvert
without the-F
option, it throws an error:psconvert [ERROR]: Modern GMT mode requires the -F option
. See https://github.com/GenericMappingTools/gmt/blob/adb244afa51ca7246cc051080c9d47193087d6c2/src/psconvert.c#L1041-L1042So since PyGMT is modern mode only, it should be fine to keep this line intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs say "If no input files are given, will convert the current active figure (see pygmt.figure). In this case, an output name must be given using parameter prefix." How does one provide other input files than the current figure using pygmt.Figure.psconvert?
If we chose not to support operations analogous to
psconvert -Tg test.ps
(which uses the same prefix as the original file name), I think we should makeprefix
required in the function signature or raise an invalid input exception to avoid the traceback below. I don't think it's obvious to that the error is caused by not usingprefix
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't, because it has not been implemented yet. The current
fig.psconvert
is a method of thepygmt.Figure()
class and is tied to thefig
, and can only 'psconvert' the active figure.Ok, I've added a try-except block in 801ba01 to raise a GMTInvalidInput if
prefix
/-F
is not given.