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 ground and airborne synthetic surveys #120

Merged
merged 28 commits into from
Nov 13, 2019
Merged

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented Nov 6, 2019

Add functions to create synthetic ground and airborne surveys based on the
gravity dataset from South Africa and the total-field magnetic anomaly dataset
from Great Britain. The synthetic surveys are samples from the original
datasets that are rescaled to the desired region defined in geodetic
coordinates.

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 the docstring, gallery, and/or tutorials.

Add functions to create synthetic ground and airborne surveys based on
the gravity dataset from South Africa and the total-field magnetic
anomaly dataset from Great Britain. The synthetic surveys are samples
from the original datasets that are projected on Cartesian coordinates
and then rescaled to the desired region.
@santisoler
Copy link
Member Author

@leouieda I'm thinking I can extend the functionalities of these two functions in order to return the synthetic surveys on either Cartesian or geodetic coordinate systems. It wouldn't be hard to implement because the original surveys are already given in geodetic. I'm thinking to add a coordinate_system argument to each function through which the user can change the desired coordinate system. What do you think?

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.

Thanks, @santisoler! These are going to be really useful. I remember that @birocoles did this in the past as well.

doc/api/index.rst Show resolved Hide resolved
harmonica/synthetic/surveys.py Show resolved Hide resolved
harmonica/synthetic/surveys.py Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
santisoler and others added 9 commits November 7, 2019 09:22
The synthetic surveys will be defined on geodetic coordinates as the
original datasets are.
Match the altitude of the survey points to the standard name for the
geodetic heights used on Harmonica.
Move the ditch of the data columns inside each survey function.
If region is None, then the synthetic survey won't be scaled.
When using the pandas.DataFrame.rename() method, a new DataFrame is
returned with the renamed columns. The original one remains intact.
Fixed wrong usage of the rename() method and merged it with the filter()
method on a single line.
Split the first tests functions in two: one that uses the default values
for region and subsection and another one that tests if scaling the
survey works as expected.
@santisoler
Copy link
Member Author

@leouieda I think I've solved all the issues.
Instead of setting the region equal to the subsection if it's None, I decided it would be better to don't scale the survey. What do you think about this?

Left some answers on the inline comments.

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.

👍 great stuff! Left a few more suggestions (mostly docs).

doc/api/index.rst Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Outdated Show resolved Hide resolved
harmonica/synthetic/surveys.py Show resolved Hide resolved
santisoler and others added 10 commits November 7, 2019 13:29
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
Co-Authored-By: Leonardo Uieda <[email protected]>
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.

👍 looks good! Just a few more comments about the docstring formats. Also, could you make two gallery examples in a separate examples/synthetic folder? Could be in a separate PR if you prefer.

The observation points are sampled from the Great Britain total-field magnetic
anomaly dataset (see :func:`harmonica.datasets.fetch_britain_magnetic`).
A portion of the original survey is cut (*data_region*) and the coordinates may be
scaled to the given *region*.
Copy link
Member

Choose a reason for hiding this comment

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

One thing to remember now is to crop these lines at 79 characters.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I added it to my TODO list, but forgot to do so.

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, is there a quick way to do this in vim? If it is, we should include it on the Contributing Guidelines. It's not an automatic way to do so, but at least it's better than nothing.
For example, I managed to do it by entering into VISUAL mode, select the lines I wanted to shrink and used gq. Nevertheless, I needed to select each block of the docstring separately instead of selecting the entire docstring and doing it only once.

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I did it the same way you did. You could write a macro but it might be more work than doing it manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we will have to do it manually then. Another option is to propose this kind of automation on Black.

Copy link
Member

Choose a reason for hiding this comment

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

The problem then would be to have black use different widths for docstrings and code. Which I doubt they would allow :(

The boundaries must be passed in the following order:
(``east``, ``west``, ``south``, ``north``, ...), defined on a geodetic
coordinate system and in degrees.
All subsequent boundaries will be ignored.
Copy link
Member

Choose a reason for hiding this comment

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

Same change as the above docstring.

Also remove unwanted dots at the end of the first line of docstrings.
@leouieda
Copy link
Member

This looks good to me 👍 Feel free to merge away when ready 🚀

@santisoler santisoler merged commit 4460807 into master Nov 13, 2019
@santisoler santisoler deleted the synthetic-surveys branch November 13, 2019 12:28
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