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 load_earth_magnetic_anomaly function for Earth magnetic anomaly dataset #2196

Merged
merged 21 commits into from
Dec 9, 2022

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented Nov 19, 2022

Add a function to import the magnetic anomaly dataset.

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 wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • 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

@willschlitzer willschlitzer added the feature Brand new feature label Nov 19, 2022
@willschlitzer willschlitzer self-assigned this Nov 19, 2022
@seisman
Copy link
Member

seisman commented Nov 19, 2022

@willschlitzer Before you start to spend more time working on this PR, do you think we should have a more general function (e.g, _load_gmt_remote_dataset) that can be reused in many load_xxx functions?

@willschlitzer
Copy link
Contributor Author

@willschlitzer Before you start to spend more time working on this PR, do you think we should have a more general function (e.g, _load_gmt_remote_dataset) that can be reused in many load_xxx functions?

@seisman Good idea. I'll get working on it (hopefully later this weekend). Should it go in something like utils.py or its own function in the datasets directory?

@seisman
Copy link
Member

seisman commented Nov 20, 2022

Should it go in something like utils.py or its own function in the datasets directory?

I think the "datasets" directory is a better place.

@seisman seisman added this to the 0.8.0 milestone Dec 1, 2022
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.

Just a few minor comments on the docstrings. Will do a more thorough review once the changes from #2200 are merged into this PR.

pygmt/datasets/__init__.py Show resolved Hide resolved
pygmt/datasets/earth_magnetic_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_magnetic_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_magnetic_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_magnetic_anomaly.py Outdated Show resolved Hide resolved
@willschlitzer willschlitzer changed the title WIP: Add function to import magnetic anomaly dataset Add function to import magnetic anomaly dataset Dec 6, 2022
@willschlitzer willschlitzer marked this pull request as ready for review December 6, 2022 14:46
pygmt/datasets/earth_magnetic_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/load_remote_dataset.py Outdated Show resolved Hide resolved
Comment on lines 61 to 65
dataset_prefix = "earth_mag_"
dataset_name = "earth_magnetic_anomaly"
grid = _load_remote_dataset(
dataset_name=dataset_name,
dataset_prefix=dataset_prefix,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm missing some context, but is there a need to declare two extra variables instead of using them directly in the function?

Suggested change
dataset_prefix = "earth_mag_"
dataset_name = "earth_magnetic_anomaly"
grid = _load_remote_dataset(
dataset_name=dataset_name,
dataset_prefix=dataset_prefix,
grid = _load_remote_dataset(
dataset_name="earth_magnetic_anomaly",
dataset_prefix="earth_mag_",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make part of this change; I'm planning on adding in mag4km in a later PR, so there will be a new parameter and some logic to set the prefix.

name="magnetic_anomaly",
long_name="Earth magnetic anomaly",
units="nT",
extra_attributes={"horizontal_datum": "WGS84"},
Copy link
Member

Choose a reason for hiding this comment

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

I'm not an expert in magnetic anomalies, but having a horizontal_datum seems strange. Is the magnetic anomaly referenced to the geoid somehow?

Also, not related to this PR, but I notice that horizontal_datum is also set as an attribute for the earth_age dataset at L96. Is seafloor crustal age also measured relative to a datum?

Copy link
Member

Choose a reason for hiding this comment

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

Please see my comment #2200 (comment).

I think horizontal_datum means the coordinate system for longitude/latitude.

Copy link

Choose a reason for hiding this comment

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

AFAIK, the datum in GMT is mainly related to the ellipsoid and its positioning. As you can see from gmt mapproject -Qd, the positioning of the ellipsoid is given as the difference in three directions between the center of the ellipsoid and the center of the earth. As to earth_age, I think it does not have height information, horizontal_datum is enough. I don't konw much about magnetic anomalies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave horizontal_datum in; per my understanding (and from the comments above) it is the system that aligns coordinates with the actual locations on Earth. So I assume it still applies to the magnetic anomaly map?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, leave it in. Thanks for the clarification everyone!

@seisman
Copy link
Member

seisman commented Dec 7, 2022

I'm wondering if you also want to add the dataset earth_mag4km in this PR.

@willschlitzer
Copy link
Contributor Author

I'm wondering if you also want to add the dataset earth_mag4km in this PR.

I was planning on adding it and an inline example in a later PR.

@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Dec 7, 2022
@seisman seisman changed the title Add function to import magnetic anomaly dataset Add load_earth_magnetic_anomaly function for Earth magnetic anomaly dataset Dec 8, 2022
@seisman seisman merged commit 2c33522 into main Dec 9, 2022
@seisman seisman deleted the load-remote-dataset/magnetic-anomaly branch December 9, 2022 01:35
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Dec 9, 2022
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
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.

4 participants