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

Merged
merged 20 commits into from
Sep 24, 2024

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 / add term "projection"
  • Make projection names consistent - identical term: following the headings of the GMT documentation

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
@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 milestone: 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.

examples/projections/misc/misc_eckertVI.py Outdated Show resolved Hide resolved
examples/projections/misc/misc_eckertIV.py Outdated Show resolved Hide resolved
examples/projections/azim/azim_general_perspective.py Outdated Show resolved Hide resolved
examples/projections/azim/azim_general_perspective.py Outdated Show resolved Hide resolved
@yvonnefroehlich
Copy link
Member Author

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

I think now all names should be consistent with the headings of the GMT documentation.

Copy link
Member

@seisman seisman 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 to me, except for two minor issues. Thanks for your patience on this PR.

examples/projections/misc/misc_eckertVI.py Outdated Show resolved Hide resolved
examples/projections/misc/misc_eckertIV.py Outdated Show resolved Hide resolved
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Sep 23, 2024
@seisman seisman merged commit 5f96b77 into main Sep 24, 2024
10 checks passed
@seisman seisman deleted the improve-proj-table branch September 24, 2024 07:14
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 24, 2024
@yvonnefroehlich
Copy link
Member Author

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.

Actually I had a similar side thought regarding the Carthesian projections some time ago.
For the Oblique Mercator projections, I just made of POC PR at #3451, to see how can look like.

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