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

Better handling of optional virtual files (e.g., shading in Figure.grdimage) #2493

Merged
merged 50 commits into from
Aug 5, 2023

Conversation

seisman
Copy link
Member

@seisman seisman commented Apr 10, 2023

Background

Some GMT modules (e.g., grdimage and grdview) can accept a primary input file and an optional input file. Here, I'll take grdimage as an example.

grdimage can accept an input grid file and an optional intensity grid for shading (-I option). Thus, in PyGMT, Figure.grdimage's shading parameter can be given in many different ways:

  1. shading=None: meaning no shading
  2. shading=True or shading=False: shading or no shading
  3. shading=0.5: a constant intensity to apply everywhere
  4. shading="+a30+nt0.8": derive the intensity from the primary input grid
  5. shading="intensity.nc": an intensity grid file
  6. shaidng="intensity.nc+a30+nt0.8": an intensity grid file plus more parameters
  7. shading=xrgrid in which xrgrid is an xarray.DataArray object
  8. more?

For cases 1-6, the shading parameter can be directly processed by the build_arg_string function, but for case 7, we need to create a virtual file for the xarray.DataArray grid.

Motivations

Since the virtual file for shading depends on the value and data type of the shading parameter, thus we have to use contextlib.ExitStack, which is designed to make it easy to work with optional context manager. However, using contextlib.ExitStack still makes the codes complicated and also make it hard to reuse the codes:

pygmt/pygmt/src/grdimage.py

Lines 182 to 195 in d2b0c39

with Session() as lib:
file_context = lib.virtualfile_from_data(check_kind="raster", data=grid)
with contextlib.ExitStack() as stack:
# shading using an xr.DataArray
if kwargs.get("I") is not None and data_kind(kwargs["I"]) == "grid":
shading_context = lib.virtualfile_from_data(
check_kind="raster", data=kwargs["I"]
)
kwargs["I"] = stack.enter_context(shading_context)
fname = stack.enter_context(file_context)
lib.call_module(
module="grdimage", args=build_arg_string(kwargs, infile=fname)
)

The main goal of this PR is to simplify the above codes to:

    with Session() as lib:
        file_context = lib.virtualfile_from_data(check_kind="raster", data=grid)
        shade_context = lib.virtualfile_from_data(check_kind="raster", data=kwargs.get("I"))
        with file_context as fname, shade_context as shade_fname:
            kwargs["I"] = shade_fname
            lib.call_module(
                module="grdimage", args=build_arg_string(kwargs, infile=fname)
            )

which is more compact and more intuitive to read.

Implementations

To achieve this, the virtualfile_from_data context manager must work for all cases. It's actually very simple. If data is not an xarray.DataArray object (cases 1-6), virtualfile_from_data should simply return a dummy context manager which does nothing but yield the argument passed to it (see the dummy_context function but also see #2491). This is done by changing the data_kind function to take arguments like None, True, False, 0.5, "+a30+nt0.8", "intensity.nc+a30+nt0.8" as a "file" (see the changes in the data_kind function).

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.
  • Use underscores (not hyphens) in names of Python files and directories.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@seisman
Copy link
Member Author

seisman commented Apr 10, 2023

Currently, all the grdimage tests still pass, which means the new codes work, but some other tests (e.g., most plot tests) fail likely caused by changes in data_kind. So need to more careful when changing data_kind.

@seisman seisman changed the title Better handling of optional virtual files (e.g., shading in Figure.grdimage) POC: Better handling of optional virtual files (e.g., shading in Figure.grdimage) Apr 10, 2023
@seisman seisman force-pushed the optional-context branch 3 times, most recently from 39af2d6 to c024041 Compare April 12, 2023 14:13
@seisman seisman added the enhancement Improving an existing feature label Apr 18, 2023
@seisman seisman added this to the 0.10.0 milestone Apr 18, 2023
@seisman seisman marked this pull request as ready for review April 26, 2023 10:01
@seisman
Copy link
Member Author

seisman commented Aug 2, 2023

@GenericMappingTools/pygmt-maintainers This PR is now completed and ready for review.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Nice work getting the if-then logic correct! Still a bit hard to follow in some parts, but I think this is the best that we can do. Just some more minor comments/suggestions and should be close to merge.

pygmt/clib/session.py Outdated Show resolved Hide resolved
pygmt/clib/session.py Outdated Show resolved Hide resolved
pygmt/helpers/utils.py Outdated Show resolved Hide resolved
pygmt/clib/session.py Outdated Show resolved Hide resolved
pygmt/clib/session.py Outdated Show resolved Hide resolved
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Excellent, thanks @seisman! Will be easier to work on #2590 after this PR is merged 😄

@weiji14 weiji14 added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Aug 4, 2023
@seisman seisman merged commit 109f209 into main Aug 5, 2023
@seisman seisman deleted the optional-context branch August 5, 2023 02:01
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants