Skip to content

Session.virtualfile_in: Deprecate parameter 'required_data' to 'required' (will be removed in v0.20.0) #3931

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.

Sign up for GitHub

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

Merged
seisman merged 6 commits into from
May 5, 2025
Merged

Session.virtualfile_in: Deprecate parameter 'required_data' to 'required' (will be removed in v0.20.0) #3931

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0c68337
Session.virtualfile_in: Deprecate parameter 'required_data' to 'requi…
seisman Apr 28, 2025
cb507a5
Merge branch 'main' into virtualfile_in/required_data
seisman May 4, 2025
c4a6a00
Update pygmt/clib/session.py
seisman May 5, 2025
532b01a
Put required before mincols
seisman May 5, 2025
4ea2ecb
Rename local variable 'required_data' to 'data_is_required'
seisman May 5, 2025
2b434e2
Reorder parameter in docstrings
seisman May 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions pygmt/clib/session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from pygmt.helpers import (
_validate_data_input,
data_kind,
deprecate_parameter,
tempfile_from_geojson,
tempfile_from_image,
)
Expand Down Expand Up @@ -1750,15 +1751,19 @@ def virtualfile_from_stringio(

# TODO(PyGMT>=0.20.0): Remove the deprecated parameter 'required_z'.
# TODO(PyGMT>=0.20.0): Remove the deprecated parameter 'extra_arrays'.
# TODO(PyGMT>=0.20.0): Remove the deprecated parameter 'required_data'.
@deprecate_parameter(
"required_data", "required", "v0.16.0", remove_version="v0.20.0"
)
def virtualfile_in( # noqa: PLR0912
self,
check_kind=None,
data=None,
x=None,
y=None,
z=None,
required=True,
mincols=2,
required_data=True,
required_z=False,
extra_arrays=None,
):
Expand All @@ -1783,9 +1788,13 @@ def virtualfile_in( # noqa: PLR0912
mincols
Number of minimum required columns. Default is 2 (i.e. require x and y
columns).
Copy link
Member

@weiji14 weiji14 May 5, 2025

Choose a reason for hiding this comment

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

Also move this docstring to below required.

required_data : bool
Set to True when 'data' is required, or False when dealing with
optional virtual files. [Default is True].
required : bool
Set to True when 'data' is required, or False when dealing with optional
virtual files. Default is True.

.. versionchanged:: v0.16.0
The parameter 'required_data' is renamed to 'required'. The parameter
'required_data' is deprecated in v0.16.0 and will be removed in v0.20.0.
required_z : bool
State whether the 'z' column is required.

Expand Down Expand Up @@ -1838,19 +1847,19 @@ def virtualfile_in( # noqa: PLR0912
)
mincols = 3

kind = data_kind(data, required=required_data)
kind = data_kind(data, required=required)
_validate_data_input(
data=data,
x=x,
y=y,
z=z,
mincols=mincols,
required_data=required_data,
required=required,
kind=kind,
)

if check_kind:
valid_kinds = ("file", "arg") if required_data is False else ("file",)
valid_kinds = ("file", "arg") if required is False else ("file",)
Comment on lines 1861 to +1862
Copy link
Member

@weiji14 weiji14 Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I feel that required by itself isn't a very descriptive parmeter name. This required_data parameter was added in #2493 to better handle optional virtual files (via the 'arg' kind). Would a name like optional_vfile (default to True) or something along those lines work? Then the check would be:

            valid_kinds = ("file", "arg") if optional_vfile is False else ("file",)

Or we could also just keep the required_data name as is? Thoughts?

Copy link
Member Author

@seisman seisman May 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would a name like optional_vfile (default to True) or something along those lines work?

You meant optional_vfile default to False?

The data_kind takes two parameters data and required. Here, required_data is renamed to required to match the parameter name in that function. If we rename it to optional_vfile, then we should use data_kind(data, required=not optional_vfile).

Copy link
Member

@weiji14 weiji14 May 4, 2025

Choose a reason for hiding this comment

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

Ah yes, I meant optional_vfile=False by default, but that's probably not a great name either. On second thought, let's go with the required parameter name then. Using just required in data_kind makes sense because we're only passing in an argument to data, and I guess we do validate whether data/x/y are not None in virtualfile_in so there shouldn't be any ambiguity, but need to update the docstring a bit.

if check_kind == "raster":
valid_kinds += ("grid", "image")
elif check_kind == "vector":
Expand Down
6 changes: 3 additions & 3 deletions pygmt/helpers/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@


def _validate_data_input( # noqa: PLR0912
data=None, x=None, y=None, z=None, mincols=2, required_data=True, kind=None
data=None, x=None, y=None, z=None, mincols=2, required=True, kind=None
) -> None:
"""
Check if the combination of data/x/y/z is valid.
Expand All @@ -53,7 +53,7 @@ def _validate_data_input( # noqa: PLR0912
>>> _validate_data_input(data="infile")
>>> _validate_data_input(x=[1, 2, 3], y=[4, 5, 6])
>>> _validate_data_input(x=[1, 2, 3], y=[4, 5, 6], z=[7, 8, 9])
>>> _validate_data_input(data=None, required_data=False)
>>> _validate_data_input(data=None, required=False)
>>> _validate_data_input()
Traceback (most recent call last):
...
Expand Down Expand Up @@ -119,7 +119,7 @@ def _validate_data_input( # noqa: PLR0912
required_z = mincols >= 3
if data is None: # data is None
if x is None and y is None: # both x and y are None
if required_data: # data is not optional
if required: # data is not optional
msg = "No input data provided."
raise GMTInvalidInput(msg)
elif x is None or y is None: # either x or y is None
Expand Down
2 changes: 1 addition & 1 deletion pygmt/src/grdfill.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ def grdfill(
# Fill mode.
with (
lib.virtualfile_in(
check_kind="raster", data=gridfill, required_data=False
check_kind="raster", data=gridfill, required=False
) as vbggrd,
lib.virtualfile_out(kind="grid", fname=outgrid) as voutgrd,
):
Expand Down
2 changes: 1 addition & 1 deletion pygmt/src/grdimage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def grdimage(self, grid: PathLike | xr.DataArray, **kwargs):
with (
lib.virtualfile_in(check_kind="raster", data=grid) as vingrd,
lib.virtualfile_in(
check_kind="raster", data=kwargs.get("I"), required_data=False
check_kind="raster", data=kwargs.get("I"), required=False
) as vshadegrid,
):
kwargs["I"] = vshadegrid
Expand Down
2 changes: 1 addition & 1 deletion pygmt/src/grdtrack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ def grdtrack(
with (
lib.virtualfile_in(check_kind="raster", data=grid) as vingrd,
lib.virtualfile_in(
check_kind="vector", data=points, required_data=False
check_kind="vector", data=points, required=False
) as vintbl,
lib.virtualfile_out(kind="dataset", fname=outfile) as vouttbl,
):
Expand Down
2 changes: 1 addition & 1 deletion pygmt/src/grdview.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def grdview(self, grid: PathLike | xr.DataArray, **kwargs):
with (
lib.virtualfile_in(check_kind="raster", data=grid) as vingrd,
lib.virtualfile_in(
check_kind="raster", data=kwargs.get("G"), required_data=False
check_kind="raster", data=kwargs.get("G"), required=False
) as vdrapegrid,
):
kwargs["G"] = vdrapegrid
Expand Down
2 changes: 1 addition & 1 deletion pygmt/src/legend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,5 @@ def legend(
raise GMTInvalidInput(msg)

with Session() as lib:
with lib.virtualfile_in(data=spec, required_data=False) as vintbl:
with lib.virtualfile_in(data=spec, required=False) as vintbl:
lib.call_module(module="legend", args=build_arg_list(kwargs, infile=vintbl))
2 changes: 1 addition & 1 deletion pygmt/src/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ def project(
y=y,
z=z,
mincols=2,
required_data=False,
required=False,
) as vintbl,
lib.virtualfile_out(kind="dataset", fname=outfile) as vouttbl,
):
Expand Down
4 changes: 1 addition & 3 deletions pygmt/src/text.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,7 @@ def text_( # noqa: PLR0912

with Session() as lib:
with lib.virtualfile_in(
check_kind="vector",
data=textfiles or data,
required_data=required_data,
check_kind="vector", data=textfiles or data, required=required_data
Copy link
Member

@weiji14 weiji14 Apr 28, 2025

Choose a reason for hiding this comment

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

At L193 and L194 above. The required_data variable wasn't changed to required?

Copy link
Member Author

@seisman seisman May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, required_data at L193-L194 is a local variable, and required is not a good name for it. Perhaps rename it to data_is_required?

Done in 4ea2ecb.

) as vintbl:
lib.call_module(
module="text",
Expand Down
Loading