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

Add alias for blockmean's -S parameter #1601

Merged
merged 12 commits into from
Nov 8, 2021
Merged

Add alias for blockmean's -S parameter #1601

merged 12 commits into from
Nov 8, 2021

Conversation

michaelgrund
Copy link
Member

Description of proposed changes

According to #1598 this PR add an alias for the -S parameter of blockmean.

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 adding new functionality, add an example to docstrings or tutorials.

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

@michaelgrund michaelgrund added the documentation Improvements or additions to documentation label Nov 1, 2021
@michaelgrund michaelgrund added this to the 0.6.0 milestone Nov 1, 2021
pygmt/src/blockm.py Outdated Show resolved Hide resolved
Co-authored-by: Will Schlitzer <schlitzer90@gmail.com>
@willschlitzer
Copy link
Contributor

@michaelgrund Sorry to leave you hanging, but I haven't had the chance to check out this branch yet!

@michaelgrund
Copy link
Member Author

@michaelgrund Sorry to leave you hanging, but I haven't had the chance to check out this branch yet!

No worries @willschlitzer 😉

@willschlitzer
Copy link
Contributor

I'm not familiar with the use of blockmean, but I'm getting the exact same output when I change the arguments to restype. I'm using the input data specified in test_blockmean_input_xyz(). Is this supposed to be the case?

@maxrjones
Copy link
Member

I'm not familiar with the use of blockmean, but I'm getting the exact same output when I change the arguments to restype. I'm using the input data specified in test_blockmean_input_xyz(). Is this supposed to be the case?

I get different results depending on the argument. Maybe try a git status to check the branch and make clean; make install?

import pygmt
data = pygmt.datasets.load_sample_bathymetry()
region = pygmt.info(data, spacing=1)
pygmt.blockmean(data=data, spacing="1m", restype="w", region=region).head(1)
longitude latitude bathymetry
245.88819 29.97895 1
pygmt.blockmean(data=data, spacing="1m", restype="m", region=region).head(1)
longitude latitude bathymetry
245.88819 29.97895 -385.0

restype is not my favorite alias for this parameter because I think truncated words are not the quickest to interpret. I would prefer something like statistic. But, it's not that strong of an opinion if @michaelgrund prefers to stick with the restype.

@michaelgrund
Copy link
Member Author

restype is not my favorite alias for this parameter because I think truncated words are not the quickest to interpret. I would prefer something like statistic. But, it's not that strong of an opinion if @michaelgrund prefers to stick with the restype.

Thanks for you comment @meghanrjones. I'm fully open for other suggestions for the alias. Indeed, statistic would fit better than restype. Another alternative would be e.g. summarize since the calculated values represent a summary of the data in each block based on different metrics (mean, count, sum, sum of weights).

@maxrjones
Copy link
Member

restype is not my favorite alias for this parameter because I think truncated words are not the quickest to interpret. I would prefer something like statistic. But, it's not that strong of an opinion if @michaelgrund prefers to stick with the restype.

Thanks for you comment @meghanrjones. I'm fully open for other suggestions for the alias. Indeed, statistic would fit better than restype. Another alternative would be e.g. summarize since the calculated values represent a summary of the data in each block based on different metrics (mean, count, sum, sum of weights).

summarize sounds good!

@seisman
Copy link
Member

seisman commented Nov 5, 2021

restype is not my favorite alias for this parameter because I think truncated words are not the quickest to interpret. I would prefer something like statistic. But, it's not that strong of an opinion if @michaelgrund prefers to stick with the restype.

Thanks for you comment @meghanrjones. I'm fully open for other suggestions for the alias. Indeed, statistic would fit better than restype. Another alternative would be e.g. summarize since the calculated values represent a summary of the data in each block based on different metrics (mean, count, sum, sum of weights).

Do you think summary (noun) is better than summarize (verb)?

@willschlitzer
Copy link
Contributor

I'm not familiar with the use of blockmean, but I'm getting the exact same output when I change the arguments to restype. I'm using the input data specified in test_blockmean_input_xyz(). Is this supposed to be the case?

I get different results depending on the argument. Maybe try a git status to check the branch and make clean; make install?

Maybe I set up my Jupyter Notebook badly, but I'm now getting different results. Looks like it works!

@michaelgrund
Copy link
Member Author

Do you think summary (noun) is better than summarize (verb)?

I'm open for this, what do the others think is the better choice?

@willschlitzer
Copy link
Contributor

Do you think summary (noun) is better than summarize (verb)?

I'm open for this, what do the others think is the better choice?

I think summary works better; summarize seems like it would make sense for a True/False argument.

pygmt/src/blockm.py Outdated Show resolved Hide resolved
pygmt/src/blockm.py Outdated Show resolved Hide resolved
pygmt/src/blockm.py Outdated Show resolved Hide resolved
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@seisman
Copy link
Member

seisman commented Nov 6, 2021

/format

pygmt/src/blockm.py Outdated Show resolved Hide resolved
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Nov 7, 2021
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
pygmt/src/blockm.py Outdated Show resolved Hide resolved
pygmt/src/blockm.py Outdated Show resolved Hide resolved
pygmt/src/blockm.py Outdated Show resolved Hide resolved
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@seisman seisman merged commit 2ebe165 into main Nov 8, 2021
@seisman seisman deleted the blockmean-add-alias branch November 8, 2021 14:14
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Nov 8, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Co-authored-by: Will Schlitzer <schlitzer90@gmail.com>
Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants