Skip to content

Conversation

@michaelgrund
Copy link
Member

@michaelgrund michaelgrund commented Mar 3, 2023

Description of proposed changes

Addresses #2390.

Preview: https://pygmt-dev--2391.org.readthedocs.build/en/2391/gallery/embellishments/timestamp.html

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

@michaelgrund michaelgrund added the documentation Improvements or additions to documentation label Mar 3, 2023
@michaelgrund michaelgrund added this to the 0.9.0 milestone Mar 3, 2023
@michaelgrund michaelgrund marked this pull request as draft March 3, 2023 08:33
@michaelgrund michaelgrund changed the title Create timestamp.py WIP: Add a gallery example for the Figure.timestamp method Mar 3, 2023
@michaelgrund michaelgrund linked an issue Mar 3, 2023 that may be closed by this pull request
@michaelgrund michaelgrund changed the title WIP: Add a gallery example for the Figure.timestamp method Add a gallery example for the Figure.timestamp method Mar 6, 2023
@michaelgrund michaelgrund marked this pull request as ready for review March 6, 2023 08:16
@michaelgrund
Copy link
Member Author

Adding a static time format string via e.g. pygmt.config(FORMAT_TIME_STAMP="2023-03-01T20:45:15") does actually not work. Any ideas why @GenericMappingTools/pygmt-maintainers ?

@michaelgrund
Copy link
Member Author

/format

@seisman
Copy link
Member

seisman commented Mar 6, 2023

Adding a static time format string via e.g. pygmt.config(FORMAT_TIME_STAMP="2023-03-01T20:45:15") does actually not work. Any ideas why @GenericMappingTools/pygmt-maintainers ?

It's because the timefmt parameter overrides the FORMAT_TIME_STAMP setting. See the relevant codes at:

kwdict, confdict={"FONT_LOGO": font, "FORMAT_TIME_STAMP": timefmt}

Comment on lines 11 to 12
# Define static format string.
pygmt.config(FORMAT_TIME_STAMP="2023-03-01T20:45:15")
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to have a static UNIX timestamp string here, because the gallery examples are not tested in the CI.

@yvonnefroehlich
Copy link
Member

yvonnefroehlich commented Mar 6, 2023

Adding a static time format string via e.g. pygmt.config(FORMAT_TIME_STAMP="2023-03-01T20:45:15") does actually not work. Any ideas why @GenericMappingTools/pygmt-maintainers ?

Hm. Probably you may want to use the text parameter of pygmt.Figure.timestamp to modify the date (please see https://www.pygmt.org/dev/api/generated/pygmt.Figure.timestamp.html#pygmt.Figure.timestamp). Please note, that using the text parameter requires GMT >=6.5.0. As I have not installed the dev version of GMT, I can not test this at the moment 🙁.

text (None or str) – If None, the current UNIX timestamp is shown in the GMT timestamp logo. Set this parameter to replace the UNIX timestamp with a custom text string instead. The text must be less than 64 characters. Requires GMT>=6.5.0.

From the upstream GMT documentation I understand that FORMAT_TIME_STAMP does affect the format, not the date or text itself (please see the upstream documentation https://docs.generic-mapping-tools.org/dev/gmt.conf.html#term-FORMAT_TIME_STAMP).

Defines the format of the time information in the UNIX time stamp. This format is parsed by the C function strftime, so that virtually any text can be used (even not containing any time information) [default is %Y %b %d %H:%M:%S].

@michaelgrund michaelgrund added the needs review This PR has higher priority and needs review. label Mar 6, 2023
@michaelgrund
Copy link
Member Author

As already suggested by @weiji14 in #2208, in my opinion the justification shortcuts are not correct:

grafik

I would say the labels must be switched across the diagonal axis. However, I think that must be fixed upstream. Maybe @PaulWessel can tell us if this is the correct behavior or it should be fixed.

@michaelgrund
Copy link
Member Author

/format

@yvonnefroehlich
Copy link
Member

yvonnefroehlich commented Mar 6, 2023

As already suggested by @weiji14 in #2208, in my opinion the justification shortcuts are not correct:
I would say the labels must be switched across the diagonal axis.

Hm. I feel they are correct. Maybe the confusion is coming from the difference between position and justification?
For the timestamp logo, only justification can be changed; the default is BL. position is fixed to BL.
For example, for an anchor point set to BR I expect everything to be left above this point, similar for the other three.
Maybe this rough sketch helps:
timestamp_anchorpoint_figure2mapplot

Edit 2023/03/11: Updated picture regarding more precise usage of "figure" and "map"/"plot".

@michaelgrund
Copy link
Member Author

michaelgrund commented Mar 6, 2023

As already suggested by @weiji14 in #2208, in my opinion the justification shortcuts are not correct:
I would say the labels must be switched across the diagonal axis.

Hm. I feel they are correct. Maybe the confusion is coming from the difference between position and justification? For the timestamp logo, only justification can be changed; the default is BL. position is fixed to BL. For example, for an anchor point set to BR I expect everything to be left above this point, similar for the other three.

Ok now I got it, thanks @yvonnefroehlich.

Comment on lines 6 to 8
can be added via the ``label`` parameter. The timestamp
will always be shown in the bottom-left corner of the
figure.
Copy link
Member

@yvonnefroehlich yvonnefroehlich Mar 6, 2023

Choose a reason for hiding this comment

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

I would be a bit more detailed here. Feel free to improve my formulation.

Suggested change
can be added via the ``label`` parameter. The timestamp
will always be shown in the bottom-left corner of the
figure.
can be added via the ``label`` parameter. The timestamp
will always be shown relative to the bottom-left corner
of the figure. By default, the ``offset`` and
``justification`` parameters are set to
``("-54p", "-54p")`` (x, y directions) and ``"BL"``
(bottom-left), respectively.

Copy link
Member

@yvonnefroehlich yvonnefroehlich Mar 11, 2023

Choose a reason for hiding this comment

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

Hm. Based on @michaelgrund's comment #2395 (comment), I am wondering whether this formulation is strictly speaking also not precise / correct and "figure" should be "map" (or "plot")?

Edit 2023/03/17: Changed in PR #2434.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps open a separate issue so that we can have more discussions.

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 issue #2437.

@michaelgrund
Copy link
Member Author

/format

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.

This PR looks good to me except the minor fix above.

@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Mar 7, 2023
Copy link
Member

@yvonnefroehlich yvonnefroehlich 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! Thanks @michaelgrund for considering all the suggestions 🙂.

@seisman seisman merged commit c974933 into main Mar 8, 2023
@seisman seisman deleted the add-timestamp-example branch March 8, 2023 08:34
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 8, 2023
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a gallery example for the Figure.timestamp method

5 participants