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 grdtrack #308

Merged
merged 17 commits into from
Mar 26, 2020
Merged

Wrap grdtrack #308

merged 17 commits into from
Mar 26, 2020

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented May 22, 2019

Description of proposed changes

Wrapping the grdtrack sampling function. Also added new tutorial function load_ocean_ridge_points() to use in test cases.

Fixes #307

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.

weiji14 added 2 commits May 22, 2019 14:28
Initial commit for #307. Implement GMT grdtrack function under sampling.py. Test cases checking proper pandas.DataFrame and xarray.DataArray inputs stored in test_grdtrack.py. Sample datasets for tests uses newly created load_east_pacific_rise_grid and load_ocean_ridge_points functions in datasets/tutorial.py.

GMT grdtrack documentation can be found at https://gmt.soest.hawaii.edu/doc/latest/grdtrack. Originally, grdtrack should take in an xyfile and -Ggridfile as parameters, and pass the output table to stdout. Here, the implementation (currently) takes in a pandas.DataFrame table and xarray.DataArray grid instead as input, and returns a pandas.DataFrame with an extra column for the sampled grid data.
Enable passing in netcdf file as input into grdtrack, instead of just xarray.DataArray. Also fix bug with kwargs being overwritten instead of appended to...
Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

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

Hi @weiji14 thanks for taking the time to implement this! I left a few minor comments. It would be great to have a gallery example of grdtrack and the new data sets as well (as simple as possible).

One thing I'm considering is if we need the East Pacific Rise grid. We can recreate that by slicing the Earth relief grids. This way we have one less function to test.

What do you think?

Rename 'table' parameter to 'points', remove default 'newcolname' parameter (require it to be explicitly set) and change the output 'ret' to 'track' in docstring. Unit tests updated accordingly.
@weiji14
Copy link
Member Author

weiji14 commented May 23, 2019

Hi @weiji14 thanks for taking the time to implement this! I left a few minor comments. It would be great to have a gallery example of grdtrack and the new data sets as well (as simple as possible).

I'll see if I have time over the weekend for making the examples (most likely in a separate pull request though). Is the sphinx gallery stable again?

One thing I'm considering is if we need the East Pacific Rise grid. We can recreate that by slicing the Earth relief grids. This way we have one less function to test.

What do you think?

That's actually a brilliant idea! I'll have a think about how to implement that in code. Hopefully I don't need to implement grdcut first 😆 Edit: Ok I've found I can do something like load_earth_relief().sel(lat=slice(-49, -42), lon=slice(-118, -107)) to get the same area.

weiji14 added 2 commits May 23, 2019 21:55
One less dataset to test by replacing the east_pacific_rise_grid with the earth_relief 60m grid (sliced to the same area). Also add tests for loading the ocean_ridge_points dataset.
PyGMT grdtrack added under "Operations on grids" (though it does require a tabular data input). Also added the ocean_ridge_points tutorial dataset used in grdtrack's unit tests and reordered the tutorial datasets alphabetically.
@weiji14 weiji14 marked this pull request as ready for review May 23, 2019 20:19
weiji14 added 2 commits May 27, 2019 21:45
Enable passing in ascii file inputs (csv, txt, etc) into grdtrack's 'points' parameter instead of just pandas.DataFrame. This requires a new 'outfile' parameter to be set. The type of 'points' input determines the type of 'track' returned, i.e. pd.DataFrame in, pd.DataFrame out; filename in, filename out. Extra unit tests created to test the various new input combinations and associated outputs.
New raster grid gallery example for grdtrack! Plotting bathymetry sampled along ocean ridge points around the globe. As this 'grid' subsection is new to the gallery, I've added it to the ExplicitOrder part within the sphinx conf.py file.
@weiji14
Copy link
Member Author

weiji14 commented May 30, 2019

@leouieda, when you have time, please review some of the changes I've made to grdtrack. I've removed the East Pacific Rise grid with the earth relief one as requested, and also enabled passing in an ASCII file input to points as requested by @seisman. Plus you'll be happy to see that there's a new gallery example! 🎉

grdtrack gallery example plot

It's been a while since I used gmt to do any plotting, and my first time using sphinx-gallery, so I really welcome any comments on how this gallery example might be improved 😄

Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

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

@weiji14 the gallery plot looks great! Thanks for all this work 🎉

I left a few comments and suggestions. Overall, this just needs a few tweaks and should be good to merge.

Using numpy.testing.assert_allclose in test_grdtrack.py unit tests and make some cosmetic changes to docs and gallery example. To make the gallery example plot clearer, grid background colormap was changed from 'ocean' to 'gray', and the points are made slightly bigger. Also changed map to be Pacific-centered!
@weiji14
Copy link
Member Author

weiji14 commented Jun 9, 2019

Ok, I've made the requested tweaks and it should be good to merge 🔀! There was a small timeout issue on the Travis CI MacOS build failing to download the earth relief 60m resolution grid 🌐, but I've restarted it and the automated checks have all passed now 🎉.

@leouieda
Copy link
Member

@weiji14 sorry I let this sit here for so long. We're working on a release candidate for GMT 6 and need to get #311 merged before we can merge this one. I'll merge it in as soon as I can.

@weiji14
Copy link
Member Author

weiji14 commented Jun 20, 2019

Please don't apologize, I'd rather have GMT6 released properly more than anything now! I see #311 has been merged so I'm gonna install 6.0.0rc1 on my linux system as well and run some tests to see if there's any issues.

Let me know once everything settles and I can merge in the changes from PyGMT master into this Pull Request :)

@weiji14
Copy link
Member Author

weiji14 commented Oct 2, 2019

Just checking in here 👋. I've merged in the latest 6.0.0rc4 stuff and the tests have passed. Would love to get this PR merged in soon as there's the "ocean ridge points" dataset that will be great from testing some of the other wrappers. Plus grdtrack will be super helpful for some of the transect/cross-section work I'm doing now.

@weiji14 weiji14 added this to the 0.1.0 milestone Oct 4, 2019
@weiji14 weiji14 requested a review from leouieda October 7, 2019 01:52
@weiji14
Copy link
Member Author

weiji14 commented Oct 15, 2019

Right, the documentation preview (from #344) is awesome! You can see the new gallery example at https://pygmt-git-fork-weiji14-sampling-grdtrack.gmt.now.sh/gallery/grid/track_sampling.html. However, there's a yellow box called 'Out' printing the stdout that comes from downloading the earth relief grid 🤦‍♂️ ...

Sampling along tracks screenshot with

Something to do with the sphinx extension?

@leouieda
Copy link
Member

@weiji14 nice! I’ll have a look at this soon. The Out box is captured by Sphinx-gallery so you can print variables etc to show values. That is fine and nothing to worry about.

@weiji14 weiji14 mentioned this pull request Oct 20, 2019
Due to new SRTM15+V2 grids, see also #350.
@weiji14
Copy link
Member Author

weiji14 commented Mar 11, 2020

@leouieda, did you have anything extra to add here? It's been more than half a year and I'd like to get on with merging this pull request (and maybe some of the other 'Wrapper' code I wrote).

Make tests pass again, by fixing the test bathymetry values, and cutting our docstrings to 79 characters instead of 88.
@weiji14
Copy link
Member Author

weiji14 commented Mar 25, 2020

Status checks for Travis not being updated properly (see https://travis-ci.community/t/github-status-not-posted-on-commits-on-repositories-using-legacy-service-integration/7798). I think we need to migrate to using the Travis Github Apps Integration. Issue opened at #403.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrapper for grdtrack
3 participants