-
Notifications
You must be signed in to change notification settings - Fork 228
Wrap filter1d #1512
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
Wrap filter1d #1512
Conversation
The style checks are getting stuck on the argument ("\s+") passed to the |
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
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.
Looks good to me except for a typo.
Co-authored-by: Dongdong Tian <[email protected]>
Hopefully at least one of @michaelgrund and @weiji14 can give this PR a final review. |
pygmt/src/filter1d.py
Outdated
@fmt_docstring | ||
@use_alias( | ||
E="end", | ||
F="filter", |
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.
filter
is a built-in function in Python, see https://docs.python.org/3/library/functions.html#filter. Sure we want to use this? Or use a parameter name like filter_type
instead?
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.
Agree with @weiji14.
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.
That makes sense to me; I'll make the change
pygmt/src/filter1d.py
Outdated
Include Ends of time series in output. The default [False] loses | ||
half the filter-width of data at each end. | ||
|
||
time_col : int or str |
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.
Is a str
allowed for time_col (-N
)? As in users can pass in the name of the time column in the pandas.DataFrame
table input?
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.
I am under the impression that GMT reads a string containing an integer or floating point number as the same value as if it was passed as a number instead. I don't know why a Python user would ever pass a number as a string, so it may just be easier to leave it as int
.
I'll see if I can try and test it, but filter1d
is one (of the many) module that I don't understand its application and just wrapped using the provided GMT example and dataset.
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.
As in users can pass in the name of the time column in the
pandas.DataFrame
table input?
I think this is a nice feature and can be implemented in a separate PR. In this initial wrapper, a time_col as int
should be good.
pygmt/src/filter1d.py
Outdated
@fmt_docstring | ||
@use_alias( | ||
E="end", | ||
F="filter", |
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.
Agree with @weiji14.
Co-authored-by: Wei Ji <[email protected]> Co-authored-by: Dongdong Tian <[email protected]>
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.
🚀
Co-authored-by: Wei Ji <[email protected]> Co-authored-by: Michael Grund <[email protected]> Co-authored-by: Dongdong Tian <[email protected]>
This pull request wraps
filter1d
.Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version