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

Wrap velo #525

Merged
merged 61 commits into from
Apr 30, 2021
Merged

Wrap velo #525

merged 61 commits into from
Apr 30, 2021

Conversation

lhoupert
Copy link
Contributor

@lhoupert lhoupert commented Jul 14, 2020

Adding the velo function! Original GMT velo function can be found at https://docs.generic-mapping-tools.org/latest/supplements/geodesy/velo.html.

Live documentation preview: https://pygmt-git-fork-lhoupert-add-velo-gmt.vercel.app/api/generated/pygmt.Figure.velo.html
Gallery example preview: https://pygmt-git-fork-lhoupert-add-velo-gmt.vercel.app/gallery/seismology/velo_arrow_ellipse.html

I followed advice from @weiji14 [see the feature request I opened here]. I implemented the function by mostly copy-modify-pasted from plot and edited the doc string. The options for velo are quite different from plot so most of the parameters were changed.

Problems encountered
I need help to understand how pygmt is handling the dataframe data. The gmt velo function requires the argument table (One or more ASCII (or binary, see -bi[ncols][type]) data table file(s) holding a number of data columns. If no tables are given then we read from standard input.). My opinion is that in pygmt, Figure.velo should reads data values from files, numpy array or panda dataframe.

Can someone help to figure out what should be the pygmt code to run the gmt example given at the end of my request?

I converted the table from standard input into a panda dataframe:

data={'Long.': {0: 0, 1: -8, 2: 0, 3: -5, 4: 5, 5: 0}, 
    ...:  'Lat.': {0: -8, 1: 5, 2: 0, 3: -5, 4: 0, 5: -5}, 
    ...:  'Evel': {0: 0, 1: 3, 2: 4, 3: 6, 4: -6, 5: 6}, 
    ...:  'Nvel': {0: 0, 1: 3, 2: 6, 3: 4, 4: 4, 5: -4}, 
    ...:  'Esig': {0: 4, 1: 0, 2: 4, 3: 6, 4: 6, 5: 6}, 
    ...:  'Nsig': {0: 6, 1: 0, 2: 6, 3: 4, 4: 4, 5: 4}, 
    ...:  'CorEN': {0: 0.5, 1: 0.5, 2: 0.5, 3: 0.5, 4: -0.5, 5: -0.5}, 
    ...:  'SITE': {0: '4x6', 1: '3x3', 2: 'NaN', 3: '6x4', 4: '-6x4', 5: '6x-4'}}  

df1 = pd.DataFrame(data)

Original GMT example for velo (more details here)
The following should make big red arrows with green ellipses outlined in red. Note that the 39% confidence scaling will give an ellipse which fits inside a rectangle of dimension Esig by Nsig.

gmt psvelo << END -h2 -R-10/10/-10/10 -W0.25p,red -Ggreen -L -Se0.2/0.39/18 \
    -B1g1 -Jx0.4/0.4 -A0.3p -P -V > test.ps
#Long. Lat. Evel Nvel Esig Nsig CorEN SITE
#(deg) (deg) (mm/yr) (mm/yr)
0. -8. 0.0 0.0 4.0 6.0 0.500 4x6
-8. 5. 3.0 3.0 0.0 0.0 0.500 3x3
0. 0. 4.0 6.0 4.0 6.0 0.500
-5. -5. 6.0 4.0 6.0 4.0 0.500 6x4
5. 0. -6.0 4.0 6.0 4.0 -0.500 -6x4
0. -5. 6.0 -4.0 6.0 4.0 -0.500 6x-4
END

test2

Fixes #510

@welcome
Copy link

welcome bot commented Jul 14, 2020

💖 Thanks for opening this pull request! 💖

Please make sure you read our contributing guidelines and abide by our code of conduct.

A few things to keep in mind:

  • If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
  • All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
  • No matter what, we are really grateful that you put in the effort to do this! 🎉

@vercel vercel bot temporarily deployed to Preview July 14, 2020 14:08 Inactive
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
@weiji14 weiji14 added the feature Brand new feature label Jul 14, 2020
@weiji14 weiji14 added this to the 0.2.x milestone Jul 14, 2020
@weiji14 weiji14 linked an issue Jul 14, 2020 that may be closed by this pull request
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.

Hi @lhoupert, thanks for opening up this Pull Request, and sorry for all the linter noise! I've made a couple of docstring related suggestions, see if they can help silence the lint errors.

To answer your question: "what should be the pygmt code to run the gmt example given", this is the code (that should go under test_velo.py)!:

"""
Tests velo.
"""
import pandas as pd

from pygmt import Figure


@pytest.mark.mpl_image_compare
def test_velo_pandas_dataframe():
    """
    Plot velocity vectors, crosses, and wedges from a pandas.DataFrame
    """
    fig = Figure()
    df = pd.DataFrame(
        data={
            "Long.": [0, -8, 0, -5, 5, 0],
            "Lat.": [-8, 5, 0, -5, 0, -5],
            "Evel": [0, 3, 4, 6, -6, 6],
            "Nvel": [0, 3, 6, 4, 4, -4],
            "Esig": [4, 0, 4, 6, 6, 6],
            "Nsig": [6, 0, 6, 4, 4, 4],
            "CorEN": [0.5, 0.5, 0.5, 0.5, -0.5, -0.5],
            # "SITE": ["4x6", "3x3", "NaN", "6x4", "-6x4", "6x-4"],
        }
    )
    fig.velo(
        data=df.to_numpy(),
        region=[-10, 10, -10, 10],
        W="0.25p,red",
        G="green",
        L=True,
        S="e0.2/0.39/18",
        frame="1g1",
        projection="x0.4/0.4",
        A="0.3p",
        V=True,
    )
    # fig.show()
    return fig

Yes, it actually works 😄, now what we need to do is to make it nicer, e.g. by:

  1. Tidy up the docstring to use long-form aliases like pen instead of W. You can preview the documentation for this PR live at https://pygmt-git-fork-lhoupert-add-velo.gmt.now.sh/api/generated/pygmt.Figure.velo.html
  2. Write some more unit tests to make sure we can handle numpy arrays, pandas DataFrames and filenames
  3. Handle string types (note how I commented out "SITE"), need to wait for Wrap GMT_Put_Strings to pass str columns into GMT C API directly #520.

Just let us know how much you're comfortable with handling and we'll try to help you with the other stuff (or you can handle everything!). If you could tick the "Allow edits from maintainers" box somewhere on the right, we can directly make commits to your branch and help you out directly too.

pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
@lhoupert
Copy link
Contributor Author

Hi @lhoupert, thanks for opening up this Pull Request, and sorry for all the linter noise! I've made a couple of docstring related suggestions, see if they can help silence the lint errors.

To answer your question: "what should be the pygmt code to run the gmt example given", this is the code (that should go under test_velo.py)!:

"""
Tests velo.
"""
import pandas as pd

from pygmt import Figure


@pytest.mark.mpl_image_compare
def test_velo_pandas_dataframe():
    """
    Plot velocity vectors, crosses, and wedges from a pandas.DataFrame
    """
    fig = Figure()
    df = pd.DataFrame(
        data={
            "Long.": [0, -8, 0, -5, 5, 0],
            "Lat.": [-8, 5, 0, -5, 0, -5],
            "Evel": [0, 3, 4, 6, -6, 6],
            "Nvel": [0, 3, 6, 4, 4, -4],
            "Esig": [4, 0, 4, 6, 6, 6],
            "Nsig": [6, 0, 6, 4, 4, 4],
            "CorEN": [0.5, 0.5, 0.5, 0.5, -0.5, -0.5],
            # "SITE": ["4x6", "3x3", "NaN", "6x4", "-6x4", "6x-4"],
        }
    )
    fig.velo(
        data=df.to_numpy(),
        region=[-10, 10, -10, 10],
        W="0.25p,red",
        G="green",
        L=True,
        S="e0.2/0.39/18",
        frame="1g1",
        projection="x0.4/0.4",
        A="0.3p",
        V=True,
    )
    # fig.show()
    return fig

Yes, it actually works 😄, now what we need to do is to make it nicer, e.g. by:

1. Tidy up the docstring to use long-form aliases like `pen` instead of `W`. You can preview the documentation for this PR live at https://pygmt-git-fork-lhoupert-add-velo.gmt.now.sh/api/generated/pygmt.Figure.velo.html

2. Write some more unit tests to make sure we can handle numpy arrays, pandas DataFrames and filenames

3. Handle string types (note how I commented out "SITE"), need to wait for #520.

Just let us know how much you're comfortable with handling and we'll try to help you with the other stuff (or you can handle everything!). If you could tick the "Allow edits from maintainers" box somewhere on the right, we can directly make commits to your branch and help you out directly too.

Thank you @weiji14 for your great suggestions. I checked and the bock "allow edits from maintainers" appears ticked.
Concerning your comments:

  1. I will use the aliases for common options. If I need to create new alias how should I do that? By looking in details, although the options in velo and plot have the same letters, they don't refer to the same thing so I think they should have different aliases no? (e.g. S, A, D, E, L, N)

  2. I ever wrote unit tests before, do you have any good doc/tutorial on this?

I am happy to give it a go and let you know when I need help. I will not be able to work on this for the next month or so, but I will be back at some point in August with some updates!

@weiji14
Copy link
Member

weiji14 commented Jul 20, 2020

Hi @lhoupert, no worries with leaving it for a while. I'm a bit busy myself these days, but I'll try to get this in for the v0.2.0 or v0.2.1 release.

I will use the aliases for common options. If I need to create new alias how should I do that? By looking in details, although the options in velo and plot have the same letters, they don't refer to the same thing so I think they should have different aliases no? (e.g. S, A, D, E, L, N)

Naming aliases is indeed a hard problem, usually we look at https://www.generic-mapping-tools.org/GMT.jl/latest/ but they don't seem to have velo, maybe just give it a go yourself and we'll 👍 or 👎.

I ever wrote unit tests before, do you have any good doc/tutorial on this?

I found https://automationpanda.com/ to be a great resource when I started writing tests (maybe 2 years ago). It's quite long but comprehensive, definitely worth a read (or skim) if you're interested into getting good at writing tests. Happy for you to take a stab at it in this PR, real world examples are the best way to learn!

@weiji14 weiji14 mentioned this pull request Jul 21, 2020
5 tasks
@weiji14 weiji14 mentioned this pull request Sep 6, 2020
11 tasks
@weiji14 weiji14 modified the milestones: 0.2.x, v0.3.x Sep 7, 2020
@lhoupert
Copy link
Contributor Author

lhoupert commented Sep 25, 2020

Hi @weiji14 , just to let you know I don't forget abouot it. Hopefully, I should have time to work on it at some point in November!

@weiji14 weiji14 self-requested a review April 30, 2021 03:24
@weiji14
Copy link
Member

weiji14 commented Apr 30, 2021

/test-gmt-dev

View at https://github.com/GenericMappingTools/pygmt/actions/runs/798234237

Copy link
Member

@michaelgrund michaelgrund left a comment

Choose a reason for hiding this comment

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

Except the minor typo, this PR looks good to me.

examples/gallery/symbols/velo_arrow_ellipse.py Outdated Show resolved Hide resolved
@weiji14
Copy link
Member

weiji14 commented Apr 30, 2021

I will merge this in tomorrow after #1232 is finalized (so that the required checks are all green). Sorry to keep the world waiting, we're a bit too perfectionist here 🤓

Note to self, cross-references: SciTools/cartopy#1487, matplotlib/matplotlib#16750 and https://stackoverflow.com/questions/60679029/cartopy-cant-plot-vector-field-with-uncertainties-and-related-questions.

@@ -0,0 +1,42 @@
"""
Copy link
Member

@seisman seisman Apr 30, 2021

Choose a reason for hiding this comment

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

Just one last-minute comment:

This gallery example should be put in the "Seismology and Geodesy" category.

Copy link
Member

Choose a reason for hiding this comment

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

You're lucky, I was just about to merge this in. Looking closely, I'm actually more tempted to put this under "Lines and vectors" as there doesn't seem to be anything seismology or geodetic about velo (though I'm not experienced in either of those fields).

Copy link
Member

Choose a reason for hiding this comment

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

The "velo" module is in the "geodesy" supplement (https://docs.generic-mapping-tools.org/dev/supplements/geodesy/velo.html), and the input columns like "eastward, northward velocity, and uncertainty of eastward, northward velocities" are also specific to geodesy applications.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, had a closer read at the Geodesy wikipedia page page and it seems to fit. I'll move the gallery example.

@weiji14 weiji14 changed the title Add velo function Wrap velo Apr 30, 2021
@weiji14 weiji14 merged commit 2081516 into GenericMappingTools:master Apr 30, 2021
@welcome
Copy link

welcome bot commented Apr 30, 2021

🎉🎉🎉 Congrats on merging your first pull request and welcome to the team! 🎉🎉🎉

Please open a new pull request to add yourself to the AUTHORS.md file. We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

@weiji14 weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Apr 30, 2021
@maxrjones
Copy link
Member

Great job getting this implemented @lhoupert and @weiji14! 🎉

@weiji14
Copy link
Member

weiji14 commented Apr 30, 2021

Thanks @lhoupert, really appreciate your work on this 🥳 You can now use fig.velo by installing the PyGMT dev version with pip, i.e.:

pip install --pre --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple pygmt

Also ping @tobiscode who might be interested to try PyGMT's velo out 😉 The velo function will also be available through normal conda/pip channels for PyGMT v0.4.0, probably by June 2021. But you can already see the gallery example live on our dev docs page:

image

Sorry again that this dragged on for so long (too many sidetracked issues), we're starting to streamline and cut down on the time to implement new features in PyGMT (<1 month), so please don't hesitate to open new feature request issues or pull requests. Feel free to provide any feedback here on GitHub or on our forum. Cheers!

@lhoupert
Copy link
Contributor Author

lhoupert commented May 1, 2021

Thanks very much @weiji14 @seisman @meghanrjones @michaelgrund and others for the achievement! And sorry to not have been available to help more at the end. I think it's going to be a great addition to pygmt particularly for the physical oceanography community :-)

@weiji14 weiji14 mentioned this pull request May 23, 2021
5 tasks
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
Add `velo` function for plotting velocity vectors, crosses, anisotropy
bars, and wedges. Original GMT velo function can be found at
https://docs.generic-mapping-tools.org/6.1/supplements/geodesy/velo.html

* clarify the type of data input
* raise error if alias S is not defined
* Refactor velo to use virtualfile_from_data and add three more unit tests
* Alias panel (c) for velo, and lightly edit some docstrings
* Alias scale (H), intensity (I) and zvalue (Z) from GMT 6.2.0

Update docstring to include new options in `velo`
from the upcoming GMT 6.2.0 that were added in
GenericMappingTools/gmt#4970,
GenericMappingTools/gmt#4901, and
GenericMappingTools/gmt#4907.
Removed the default vector="+p1p+e" argument, and
formatted docstring to have nested lists to be nicer looking.

* Fix typos and add intersphinx mappings
* Update the frame, region, projection and color of velo example
* Rename alias intensity for -I to shading
* Rename alias facecolor for -G to color
* Move docstring for projection (J) and region (R) up a bit
* Rename alias to spec (S) and update baseline images for GMT 6.2.0rc1
* Mark fig.velo tests with xfail
* Rename alias uncertainty_color to uncertaintycolor for parameter E
* Move velo_arrow_ellipse.py under "Seismology and Geodesy" category

Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Michael Grund <[email protected]>
Co-authored-by: Meghan Jones <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Brand new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Psvelo [Plot velocity vectors, crosses, and wedges]
8 participants