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

Remote datasets: Include images of the official documentation in API reference #2728

Merged
merged 23 commits into from
Oct 12, 2023

Conversation

yvonnefroehlich
Copy link
Member

@yvonnefroehlich yvonnefroehlich commented Oct 9, 2023

Description of proposed changes

This PR aims to include images of the remote datasets in the API reference by adding the links to the images showen at https://www.generic-mapping-tools.org/remote-datasets/.

Fixes #2325

Preview:

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.
  • Use underscores (not hyphens) in names of Python files and directories.

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

@yvonnefroehlich yvonnefroehlich added the documentation Improvements or additions to documentation label Oct 9, 2023
@yvonnefroehlich yvonnefroehlich added this to the 0.11.0 milestone Oct 9, 2023
@yvonnefroehlich yvonnefroehlich self-assigned this Oct 9, 2023
@yvonnefroehlich
Copy link
Member Author

yvonnefroehlich commented Oct 9, 2023

Some questions:

pygmt/datasets/earth_age.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member

seisman commented Oct 9, 2023

  • For now, I placed the images just above the Parameters section. Or do people prefer another position? Maybe above the Refer to xyz sentence?

I prefer to put the images at the top, just below the "Load xxx dataset in various resolutions" sentence, because the images are more eye-catching than the long description.

I also prefer to make the images slightly smaller. I usually use a relative width like width: 80% which is 80% of the content width rather than an absolute width like width: 700px.

They look quite different in some places. A side-by-side table sounds a good idea.

I think the colormap really doesn't matter much for earth_mask.
Unlike other images, the earth_mask image has a frame and annotations, which doesn't look good. I think we should open an upstream issue to fix it.

@yvonnefroehlich yvonnefroehlich added the skip-changelog Skip adding Pull Request to changelog label Oct 9, 2023
@yvonnefroehlich
Copy link
Member Author

yvonnefroehlich commented Oct 9, 2023

I prefer to put the images at the top, just below the "Load xxx dataset in various resolutions" sentence, because the images are more eye-catching than the long description.

That's fine with me and is done in commit fdfa599.

I also prefer to make the images slightly smaller. I usually use a relative width like width: 80% which is 80% of the content width rather than an absolute width like width: 700px.

Using a relative width is also fine with me.
However, there seem to be no big difference in the size of the images between using 700 px and 80 %.
Update: I changed the remaining images to use a relative width in commit 8c0b986.

They look quite different in some places. A side-by-side table sounds a good idea.

A table containing the images for the magnetic datasets is added in commits 111b0c1, 9fb1f13, and 8399c28.

I think the colormap really doesn't matter much for earth_mask. Unlike other images, the earth_mask image has a frame and annotations, which doesn't look good. I think we should open an upstream issue to fix it.

Yeah, I was also wondering about the annotated frame and think this image should be consistent with the images for the other datasets.

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@seisman
Copy link
Member

seisman commented Oct 10, 2023

The following warning is from pylint:

pygmt/datasets/earth_magnetic_anomaly.py:30:0: C0301: Line too long (102/100) (line-too-long)

and the following warning is from pycodestyle:

pygmt/datasets/earth_magnetic_anomaly.py:28:80: W505 doc line too long (119 > 79 characters) [pycodestyle]

It seems we have two tools that check line length. Maybe we should disable one?

BTW, the following code can disable both warnings but itself is too long:

# pylint: disable=line-too-long # noqa: W505

@yvonnefroehlich
Copy link
Member Author

The following warning is from pylint:

pygmt/datasets/earth_magnetic_anomaly.py:30:0: C0301: Line too long (102/100) (line-too-long)

and the following warning is from pycodestyle:

pygmt/datasets/earth_magnetic_anomaly.py:28:80: W505 doc line too long (119 > 79 characters) [pycodestyle]

It seems we have two tools that check line length. Maybe we should disable one?

BTW, the following code can disable both warnings but itself is too long:

# pylint: disable=line-too-long # noqa: W505

If we us a tool setting a limit of < 79 signs, I do not see the need or sense of having an additional tool for checking for < 100 signs. So, if we would decide to not use pylint's limit < 100 anymore, we would only have the limit < 79 from pycodestyle and this warning could be disabled via # noqa: W505. Then it should be possible to have the link in one line, correct?

@yvonnefroehlich
Copy link
Member Author

I think the colormap really doesn't matter much for earth_mask. Unlike other images, the earth_mask image has a frame and annotations, which doesn't look good. I think we should open an upstream issue to fix it.

Yeah, I was also wondering about the annotated frame and think this image should be consistent with the images for the other datasets.

I opened an issue regarding removing the annotated fancy frame at GenericMappingTools/remote-datasets#83.

@seisman
Copy link
Member

seisman commented Oct 10, 2023

The following warning is from pylint:

pygmt/datasets/earth_magnetic_anomaly.py:30:0: C0301: Line too long (102/100) (line-too-long)

and the following warning is from pycodestyle:

pygmt/datasets/earth_magnetic_anomaly.py:28:80: W505 doc line too long (119 > 79 characters) [pycodestyle]

It seems we have two tools that check line length. Maybe we should disable one?
BTW, the following code can disable both warnings but itself is too long:

# pylint: disable=line-too-long # noqa: W505

If we us a tool setting a limit of < 79 signs, I do not see the need or sense of having an additional tool for checking for < 100 signs. So, if we would decide to not use pylint's limit < 100 anymore, we would only have the limit < 79 from pycodestyle and this warning could be disabled via # noqa: W505. Then it should be possible to have the link in one line, correct?

Done in PR #2735.

@seisman
Copy link
Member

seisman commented Oct 11, 2023

PR #2735 has been merged, now it's possible to write like:

.. figure:: https://long/path/to/image.png # noqa: W505

pygmt/datasets/earth_age.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_free_air_anomaly.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_geoid.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_mask.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_relief.py Outdated Show resolved Hide resolved
pygmt/datasets/earth_vertical_gravity_gradient.py Outdated Show resolved Hide resolved
…re::'

Co-authored-by: Dongdong Tian <seisman.info@gmail.com>
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Oct 11, 2023
@seisman seisman merged commit 019d13e into main Oct 12, 2023
16 checks passed
@seisman seisman deleted the add-link-image-datasets branch October 12, 2023 00:24
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Oct 12, 2023
@seisman
Copy link
Member

seisman commented Oct 14, 2023

@yvonnefroehlich See GenericMappingTools/remote-datasets#89 for the upstream changes of image names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default CPTs for GMT remote datasets
2 participants