Skip to content
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

Reduce repeated code for managing the outgrid parameter #1400

Closed
maxrjones opened this issue Jul 30, 2021 · 2 comments · Fixed by #1439
Closed

Reduce repeated code for managing the outgrid parameter #1400

maxrjones opened this issue Jul 30, 2021 · 2 comments · Fixed by #1439
Assignees
Labels
enhancement Improving an existing feature maintenance Boring but important stuff for the core devs
Milestone

Comments

@maxrjones
Copy link
Member

maxrjones commented Jul 30, 2021

Description of the desired feature

The block of code below is currently used seven times throughout PyGMT modules (probably more pending Will's PRs). Based on GenericMappingTools/gmt#5563 (comment), it may need to be used up to 34 times if we continue adding functions 1:1 with GMT modules. I think it would be useful to add an build_outgrid_args() method (or something similar) that can be used throughout to build the argument string using either outgrid or tempfile.name. I expect this would be simplest to implement as a new method in the Session class, although it could be a function in utils.

if "G" not in kwargs.keys(): # if outgrid is unset, output to tempfile
kwargs.update({"G": tmpfile.name})
outgrid = kwargs["G"]
arg_str = " ".join([infile, build_arg_string(kwargs)])

Edit: The original post only mentioned managing the input arguments to call_module, but the same can be applied to the output for the current default behavior of returning a DataArray if outgrid is not set:

if outgrid == tmpfile.name: # if user did not set outgrid, return DataArray
with xr.open_dataarray(outgrid) as dataarray:
result = dataarray.load()
_ = result.gmt # load GMTDataArray accessor information
else:
result = None # if user sets an outgrid, return None

Are you willing to help implement and maintain this feature? Yes

@maxrjones maxrjones added question Further information is requested maintenance Boring but important stuff for the core devs enhancement Improving an existing feature labels Jul 30, 2021
@weiji14
Copy link
Member

weiji14 commented Aug 2, 2021

Yes, probably best to standardize this chunk if it's going to be used up to 34 times (!!!). Personally I wouldn't hide away the if "G" not in kwargs.keys(): since there might be extra logic involved, e.g. at

pygmt/pygmt/src/grdview.py

Lines 127 to 138 in b2808cf

if "G" in kwargs: # deal with kwargs["G"] if drapegrid is xr.DataArray
drapegrid = kwargs["G"]
if data_kind(drapegrid) in ("file", "grid"):
if data_kind(drapegrid) == "grid":
drape_context = lib.virtualfile_from_grid(drapegrid)
kwargs["G"] = stack.enter_context(drape_context)
else:
raise GMTInvalidInput(
f"Unrecognized data type for drapegrid: {type(drapegrid)}"
)
fname = stack.enter_context(file_context)
arg_str = " ".join([fname, build_arg_string(kwargs)])

But certainly for the latter if outgrid == tmpfile.name block, we can have a function producing the correct output grid format. This can be called in a PyGMT module at the end using return some_grid_function_name(result). Maybe start a new pygmt/io.py file to put all of PyGMT's Input/Output logic? Will tie in nicely with the table-output discussion happening at #1318, and could also be a good place longer-term to add in logic for the expanding variety of PyGMT input formats (xref #949).

@weiji14 weiji14 added this to the 0.5.0 milestone Aug 2, 2021
@maxrjones
Copy link
Member Author

Cool, I can likely find time to work on this in late August if no one else posts that they would like to work on the issue before then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature maintenance Boring but important stuff for the core devs
Projects
None yet
2 participants