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

DOC/projections: Simplify links in the projection table to use directly the titles of the examples #3407

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

yvonnefroehlich
Copy link
Member

@yvonnefroehlich yvonnefroehlich commented Aug 21, 2024

Description of proposed changes

See #3405 (comment) for context.

  • Correct link in projection table to projection code A (the first row)
  • Simply link in projection table and use titles of examples
  • Make projection names consistent - identical case: lower-case
  • Make projection names consistent - keep or remove term "projection" -> separate PR
  • Make projection names consistent - identical term: following the GMT table -> separate PR

Related upstream GMT PR: GenericMappingTools/gmt#8567

Preview

Fixes #3405

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 command is:

  • /format: automatically format and lint the code

@yvonnefroehlich yvonnefroehlich added documentation Improvements or additions to documentation maintenance Boring but important stuff for the core devs enhancement Improving an existing feature skip-changelog Skip adding Pull Request to changelog and removed enhancement Improving an existing feature labels Aug 21, 2024
doc/techref/projections.md Outdated Show resolved Hide resolved
doc/techref/projections.md Outdated Show resolved Hide resolved
@seisman seisman added this to the 0.13.0 milestone Aug 21, 2024
@@ -1,6 +1,6 @@
r"""
Mollweide
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 unsure about changes like this. According to https://en.wikipedia.org/wiki/List_of_map_projections, the official name for this projection should be "Mollweide" rather than "Mollweide equal-area". "equal-area" is just a property of this projection, not its name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was unsure about this change. I used the upstream GMT table as an orientation. I fell it would be good, if both tables use the identical name for the projections. Maybe it is better to remove the "equal-area" and update the GMT table?

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for the upstream change first.

Copy link
Member Author

Choose a reason for hiding this comment

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

So the GMT-related PR is merged. Let's continue here.
Do we want to make the projection names consistent with GMT or consistent between the overview table and the examples? For the later one, do we want to use the name from the GMT projection table or form the headings of the GMTdocumentation page?

Copy link
Member

@seisman seisman Sep 22, 2024

Choose a reason for hiding this comment

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

I think we should make the projection names consistent with headings of the GMT documentation.

@yvonnefroehlich yvonnefroehlich changed the title DOC/projections: Simplify links in projection table and make projection name consistent WIP: DOC/projections: Simplify links in projection table and make projection name consistent Aug 22, 2024
@yvonnefroehlich yvonnefroehlich self-assigned this Aug 31, 2024
@yvonnefroehlich yvonnefroehlich mentioned this pull request Aug 31, 2024
39 tasks
@seisman
Copy link
Member

seisman commented Sep 3, 2024

Not sure if we can finish the upstream PR GenericMappingTools/gmt#8567 and this PR in time. If not, at least we can fix the link to the projection code A in PR #3370, and then work on the projection table after v0.13.0. @yvonnefroehlich What do you think?

@yvonnefroehlich
Copy link
Member Author

yvonnefroehlich commented Sep 3, 2024

Not sure if we can finish the upstream PR GenericMappingTools/gmt#8567 and this PR in time. If not, at least we can fix the link to the projection code A in PR #3370, and then work on the projection table after v0.13.0. @yvonnefroehlich What do you think?

Yes, unfortunately it looks like I am running a bit out of time with these PRs (just back to work since this Monday). For now, I fixed the like to the projection code A in the common typo fixes PR #3370.

@yvonnefroehlich
Copy link
Member Author

yvonnefroehlich commented Sep 4, 2024

Not sure if we can finish the upstream PR GenericMappingTools/gmt#8567 and this PR in time. If not, at least we can fix the link to the projection code A in PR #3370, and then work on the projection table after v0.13.0. @yvonnefroehlich What do you think?

Yes, unfortunately it looks like I am running a bit out of time with these PRs (just back to work since this Monday). For now, I fixed the like to the projection code A in the common typo fixes PR #3370.

Maybe we can split the PR into two parts: (i) simplifying the links (this PR, and completed for v0.13.0) and (ii) making the projection names consistent (new PR, and moved to v0.14.0).

I have reverted the changes of the projection names (see fb2e904). For now, the names are only changed regarding using consistently lower-case letters and removing the term "projection" (commit 46b37b7).

@yvonnefroehlich yvonnefroehlich added the needs review This PR has higher priority and needs review. label Sep 4, 2024
@yvonnefroehlich yvonnefroehlich changed the title WIP: DOC/projections: Simplify links in projection table and make projection name consistent DOC/projections: Simplify links in the projection table to use directly the titles of the examples Sep 4, 2024
@seisman
Copy link
Member

seisman commented Sep 4, 2024

Maybe we can split the PR into two parts: (i) simplifying the links (this PR, and completed for v0.13.0) and (ii) making the projection names consistent (new PR, and moved to v0.14.0).

Sounds good to me.

I have reverted the changes of the projection names (see fb2e904). For now, the names are only changed regarding using consistently lower-case letters and removing the term "projection" (commit 46b37b7).
I'm debating if we should keep the "projection" term instead. For some projections like Mercator or Hammer, the term "projection" makes no difference, but for some projections like "Polyconic", "Polyconic" is an adjective, and "Polyconic projection" may make more sense in the example title.

@yvonnefroehlich
Copy link
Member Author

yvonnefroehlich commented Sep 4, 2024

I have reverted the changes of the projection names (see fb2e904). For now, the names are only changed regarding using consistently lower-case letters and removing the term "projection" (commit 46b37b7).

I'm debating if we should keep the "projection" term instead. For some projections like Mercator or Hammer, the term "projection" makes no difference, but for some projections like "Polyconic", "Polyconic" is an adjective, and "Polyconic projection" may make more sense in the example title.

Yes, I was actually wondering about this. But as we currently have "Azimuthal equistant", "General stereographic", "Gnomonic", "Orthographic", "Equidistant conic", and "Sinusoidal" without the term "projection" and these are also adjectives and sound (at least for me) better with "projection", I removed it for "Polyconic" and "Lambert conic conformal" to make it consistent. Maybe we can include this in the projection names discussion and decide on the GMT docs first (see also comment GenericMappingTools/gmt#8567 (comment)) and do the same for the PyGMT docs?

@seisman seisman modified the milestones: 0.13.0, 0.14.0 Sep 5, 2024
@yvonnefroehlich
Copy link
Member Author

I have reverted the changes of the projection names (see fb2e904). For now, the names are only changed regarding using consistently lower-case letters and removing the term "projection" (commit 46b37b7).

I'm debating if we should keep the "projection" term instead. For some projections like Mercator or Hammer, the term "projection" makes no difference, but for some projections like "Polyconic", "Polyconic" is an adjective, and "Polyconic projection" may make more sense in the example title.

Yes, I was actually wondering about this. But as we currently have "Azimuthal equistant", "General stereographic", "Gnomonic", "Orthographic", "Equidistant conic", and "Sinusoidal" without the term "projection" and these are also adjectives and sound (at least for me) better with "projection", I removed it for "Polyconic" and "Lambert conic conformal" to make it consistent. Maybe we can include this in the projection names discussion and decide on the GMT docs first (see also comment GenericMappingTools/gmt#8567 (comment)) and do the same for the PyGMT docs?

Hm. I now reverted all projection name changes in this PR.

I have reverted the changes of the projection names (see fb2e904). For now, the names are only changed regarding using consistently lower-case letters and removing the term "projection" (commit 46b37b7).

I'm debating if we should keep the "projection" term instead. For some projections like Mercator or Hammer, the term "projection" makes no difference, but for some projections like "Polyconic", "Polyconic" is an adjective, and "Polyconic projection" may make more sense in the example title.

Yes, I was actually wondering about this. But as we currently have "Azimuthal equistant", "General stereographic", "Gnomonic", "Orthographic", "Equidistant conic", and "Sinusoidal" without the term "projection" and these are also adjectives and sound (at least for me) better with "projection", I removed it for "Polyconic" and "Lambert conic conformal" to make it consistent. Maybe we can include this in the projection names discussion and decide on the GMT docs first (see also comment GenericMappingTools/gmt#8567 (comment)) and do the same for the PyGMT docs?

Hm. I am currently unsure what still has to be done in this PR. Should I revert all changes of the projection names, including the change to lower-case? Or should I only add the term "projection" back for for "Polyconic" and "Lambert conic conformal"?

@yvonnefroehlich yvonnefroehlich added this to the 0.13.0 milestone Sep 5, 2024
@seisman
Copy link
Member

seisman commented Sep 5, 2024

I prefer to leave this PR to v0.14.0 so that we can have more comments/thoughts from others before we make more changes.

@yvonnefroehlich
Copy link
Member Author

yvonnefroehlich commented Sep 5, 2024

I prefer to leave this PR to v0.14.0 so that we can have more comments/thoughts from others before we make more changes.

That's fine with me. Just thought we split this PR and want to get to link simplication in v0.13.0.

@yvonnefroehlich yvonnefroehlich modified the milestones: 0.13.0, 0.14.0 Sep 5, 2024
@seisman seisman removed the needs review This PR has higher priority and needs review. label Sep 11, 2024
@yvonnefroehlich
Copy link
Member Author

\format

@seisman
Copy link
Member

seisman commented Sep 22, 2024

Is this PR finished? I haven't checked all changes carefully, but generally they look good.

One off-topic question: Do you think we should combine the three gallery examples for the Oblique Mercator projections (https://www.pygmt.org/dev/projections/cyl/cyl_oblique_mercator_1.html, https://www.pygmt.org/dev/projections/cyl/cyl_oblique_mercator_2.html, https://www.pygmt.org/dev/projections/cyl/cyl_oblique_mercator_3.html) into the same page, since they are just one projection with different ways to specify the projection parameters.

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 maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the "GMT Map Projections" table
2 participants