-
Notifications
You must be signed in to change notification settings - Fork 224
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 wiggle #1145
Wrap wiggle #1145
Conversation
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.
Hi @core-man, just reviewed your implementation of wiggle
and it looks pretty good! Only a few minor comments for now. Let us know if you need help with adding a unit test, you could just adapt your gallery example into a unit test (but simplify it as much as possible, take out unnecessary arguments like frame
perhaps).
Oh, and just to prevent this PR from taking up too much time (speaking from experience), please don't wrap every single alias for wiggle
right now, just keep it to the bare minimum. We can complete the long alias list in a separate PR.
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Oh, and please remember to add |
Thank you. Will Ping you when I need help. I am preparing for the job interview these two weeks. Will come back soon next week.
Good to know. I thought to wrap all the arguments in one PR. |
Co-authored-by: Michael Grund <23025878+michaelgrund@users.noreply.github.com>
@michaelgrund Thanks for the review. |
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'm happy with the def wiggle()
code implementation and single unit test, and you've wrapped the main required parameter scale (Z) and a couple of other basic ones which is good. Just a few minor edits and it should be 🤞 close to merging.
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@@ -0,0 +1,38 @@ | |||
""" |
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 don't think wiggle
is a seismology-specific module. I recommend moving the example to the "Line" category.
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: Dongdong Tian <seisman.info@gmail.com>
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 @seisman to move the gallery example to the "Line" category since e.g. traces of active reflection seismic data are also often plotted in such way (and not only seismology related content). Except that point, I'm fine with that PR. Great work!
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com> Co-authored-by: Dongdong Tian <seisman.info@gmail.com> Co-authored-by: Michael Grund <23025878+michaelgrund@users.noreply.github.com>
Description of proposed changes
Wrap wiggle.
The following special alias is added:
position
track
scale
A simple gallery example revised from GMT examples is added.
A simple test revised from the example above is added.
To be updated:
Some common alias needs to be added, e.g., t (
transparency
). Note thatcolumn
will be renamed toincols
.Some special alias needs to be added, e.g., A, C, F, I
Fixes #1114
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