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

Tutorial about how grid coordinates are generated #192

Merged
merged 16 commits into from
Jul 23, 2019
Merged

Tutorial about how grid coordinates are generated #192

merged 16 commits into from
Jul 23, 2019

Conversation

jessepisel
Copy link
Contributor

I created a tutorial for grid generation that covers region and spacing adjustment and pixel registration. Let me know if this is what you had in mind for this tutorial and I can change as needed. Addresses #161

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • [na ] Add tests for new features or tests that would have caught the bug that you're fixing.
  • [na] Add new public functions/methods/classes to doc/api/index.rst and verde/__init__.py.
  • [na] Write detailed docstrings for all functions/classes/methods.
  • If adding new functionality, add an example to the docstring, gallery, and/or tutorials.

jessepisel and others added 6 commits April 29, 2019 10:44
syncing fork to verde master
initial tutorial on how grid_coordinates works. Work in progress, still need to add pixel registration, lint, format, and build with sphinx
added pixel_registration section. still needs formatting, linting etc.
formatted, still needs linting
@leouieda
Copy link
Member

Thanks, Jesse! I'll have a look at this soon. Meanwhile, @santisoler could you have a look at the tutorial since you're not as familiar with the implementation? You might spot something that I won't.

Copy link
Member

@santisoler santisoler left a comment

Choose a reason for hiding this comment

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

Thanks @jessepisel for tackling this!
I added some inline comments below, most of them are just typos or fixing missing Markdown style.

I like how you show a problematic choice of region and spacing and how the two available approaches (adjust spacing or region) solve it.
I would like to introduce the tutorial with a non problematic region and spacing (e.g. w, e, s, n = 0, 1000, 0, 1000 and spacing = 100) just to show what grid_coordinates creates. And then show the one you proposed. What do you think?

Regarding the plots, have you tried adding some transparency to the points? Doing so may help visualizing if the two grids overlap. Besides, how do you feel adding a grid to the plots, maybe with the same spacing as one of the computed grids? I think it would help to visualize the slight differences between both grids.

Regarding the pixel registration, I would keep the region or spacing adjust aside. I would choose a non-problematic combination of region and spacing and show how the differences between grids created with pixel_register = True and pixel_register = False. I'm just thinking to focus on what pixel registration does independently of the previous adjustment.

Take these comments as mere suggestions. We could continue discussing how to improve this. I'm sure @leouieda's point of view will be very enlightening.

tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
Co-Authored-By: Santiago Soler <santiago.r.soler@gmail.com>
@jessepisel
Copy link
Contributor Author

@santisoler Thank you so much for the review! I accepted all the typos and missing markdown styling changes you made.
For the rest of your comments:

I would like to introduce the tutorial with a non problematic region and spacing (e.g. w, e, s, n = 0, 1000, 0, 1000 and spacing = 100) just to show what grid_coordinates creates. And then show the one you proposed. What do you think?

I like this idea, I think that if it starts with an easy to understand and non problematic spacing it would give a better understanding of what grid_coordinates is doing.

Regarding the plots, have you tried adding some transparency to the points? Doing so may help visualizing if the two grids overlap. Besides, how do you feel adding a grid to the plots, maybe with the same spacing as one of the computed grids? I think it would help to visualize the slight differences between both grids.

I think transparency on the points would help a ton, and grids would be really useful for visualizing the grid. Great suggestion!

Regarding the pixel registration, I would keep the region or spacing adjust aside. I would choose a non-problematic combination of region and spacing and show how the differences between grids created with pixel_register = True and pixel_register = False. I'm just thinking to focus on what pixel registration does independently of the previous adjustment.

This would be great to show the difference between pixel registration and normal gridding. It should help people on that front as well.

I will update this later this week and commit a new version after I have made your suggested changes!

@santisoler
Copy link
Member

I'm glad you found my suggestions useful!
Let me know if you need help or more reviewing. 👍

@jessepisel
Copy link
Contributor Author

@santisoler I updated the tutorial with an initial non-problematic grid, and added some grid lines to the other plots. Will you take a look at it and see if it makes sense the way it laid out?

Copy link
Member

@santisoler santisoler left a comment

Choose a reason for hiding this comment

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

Great job @jessepisel! I like it very much now! I just proposed some changes regarding the organization of the first examples. Check them out and let me know what do you think about them.

Btw, I love the last figure!

One last thought. Last week I was working with some Equivalent Layer gridding and using grid_coordinates. I noticed that most of the times when working with potential field modelling you need a heights coordinates besides the easting and northing ones. So I think it would be nice to add an example on how the extra_coords argument works and in what cases it may be useful (you could put as an example creating a grid at 1km height above the sea level). What do you think?

tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
@jessepisel
Copy link
Contributor Author

Thanks @santisoler I appreciate the review. I like all of your changes for the organization, I think it makes sense working through it. The headers for each section fit nicely as well. I also added the section on extra_coords, I totally missed that when I was going through the doc string. I added a short section with extra_coords for both a height and a time. Thanks for the review again, take a look at what I changed and let me know if it works!

"""
.. _grid_coordinates:

Gridding coordinates
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Gridding coordinates
Grid coordinates

########################################################################################
# Adjusting region boundaries when creating the grid
# --------------------------------------------------
# Now let's create a region that is 1555 units west-east, and 1250 units south-north
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to keep the region the same as the previous example and modify the spacing slightly so that it doesn't divide exactly anymore. This way, between the last example and this one only 1 thing changes (keeping the cognitive burden low).

@leouieda
Copy link
Member

@jessepisel thanks for all this great work! There are changes now in the master branch that fix all those failing tests. Let me know when you're ready and we can merge in the master branch here.

@jessepisel
Copy link
Contributor Author

@leouieda sounds great! I have an updated version of the tutorial that addresses your comments. I am having some errors when I build the documentation, should I commit and then merge master, or merge master and then commit the new changes?

@leouieda
Copy link
Member

@jessepisel feel free to commit your changes and then merge master. In general, feel free to commit as much as you want. When we merge the pull request, all commits get squashed down into one and we rewrite the commit message based on the PR description. So the history will always be clean even if you have many broken commits in the PR.

@jessepisel
Copy link
Contributor Author

@leouieda Perfect. I have added the latest commit and merged from master. Good to know the commit messages are rewritten when the PR is merged, I wasn't sure what the process was for message history. Thank you again for all the help!

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.

@jessepisel a few minor tweaks and we should be good to go! Another thing you'll need to do is put this tutorial in the table of contents. Place an entry for it in doc/index.rst between sample_data/index.rst and tutorials/trends.rst so that it's the second thing in our User Guide.

tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
tutorials/grid_coordinates.py Outdated Show resolved Hide resolved
jessepisel and others added 3 commits July 17, 2019 08:22
Co-Authored-By: Leonardo Uieda <leouieda@gmail.com>
Co-Authored-By: Leonardo Uieda <leouieda@gmail.com>
@leouieda leouieda changed the title Grid coordinates tutorial WIP Tutorial about how grid coordinates are generated Jul 17, 2019
@leouieda
Copy link
Member

@jessepisel great work! This is ready to merge on my part.

@santisoler do you have anything else to add? If not, we can merge this in and release v1.2.0.

@leouieda leouieda merged commit 0332da4 into fatiando:master Jul 23, 2019
@leouieda
Copy link
Member

Merging this in. Thanks again @jessepisel!

@jessepisel jessepisel deleted the grid_coordinates_docs branch November 23, 2019 15:25
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.

3 participants