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 Figure.timestamp to plot the GMT timestamp logo #2208

Merged
merged 44 commits into from
Mar 3, 2023
Merged

Add Figure.timestamp to plot the GMT timestamp logo #2208

merged 44 commits into from
Mar 3, 2023

Conversation

seisman
Copy link
Member

@seisman seisman commented Nov 24, 2022

Description of proposed changes

In this PR, I add the Figure.timestamp method to plot the GMT timestamp logo, just like what GMT's -U option does. The final goal is to remove the timestamp alias from all the plotting methods (c.f. #2135 and #924 (comment)).

Here are some notes:

  1. The full syntax of the -U option is: -U[<label>][+c][+j<just>][+o<dx>[/<dy>]]. -U+c can be used to append the raw GMT command (e.g., gmt coast -R... -J...) after the timestamp logo. It's not implemented because (1) It's rarely used. A figure is usually produced by multiple commands, so it makes no sense to show the command string of a single command; (2) It makes little sense to show the raw GMT command string for a figure produced by a Python script; (3) It's also impossible to implement it in the Figure.timestamp() function.
  2. The Figure.timestamp method is implemented by calling gmt plot -T -U...
  3. There are four GMT defaults (FONT_LOGO, MAP_LOGO, MAP_LOGO_POS, and FORMAT_TIME_STAMP) that can affect the timestamp logo.
    • MAP_LOGO controls if a timestamp logo should be plotted. It makes no sense here, so ignored.
    • MAP_LOGO_POS defaults to BL/-54p/-54p. It's equivalent to setting the +j and +o modifiers. So it's ignored.
    • FONT_LOGO can control the font ID of the text strings. It's implemented by passing --FONT_LOGO=xxx to the GMT C API.
    • FORMAT_TIME_STAMP can control the format of the timestamp. It's implemented by passing --FORMAT_TIME_STAMP to the GMT C API.
  4. An upstream bug of the +o modifier for GMT <=6.4.0 (-U doesn't work with a single offset value gmt#7107).
  5. For GMT<=6.4.0, FONT_LOGO can only change the font ID, but since GMT 6.5.0, it can also change both the font ID and color (but not the font size). See Allow FONT_LOGO to change font color gmt#7125
  6. In GMT 6.5.0, a new modifier +t will be added. See Allow FONT_LOGO to change font color gmt#7125.
  7. It's difficult to test this function, because the image has the timestamp string, which changes every second. The workaround here is passing a constant format string to FORMAT_TIME_STAMP (e.g., --FORMAT_TIME_STAMP="9999-99-99T99:99:99").

Examples:

>>> # Plot the GMT timestamp logo.
>>> import pygmt
>>> fig = pygmt.Figure()
>>> fig.timestamp()
>>> fig.show()

>>> # Plot the GMT timestamp logo with a custom label.
>>> fig = pygmt.Figure()
>>> fig.timestamp(label="Powered by PyGMT")
>>> fig.show()

1
2

Documentation preview: https://pygmt-dev--2208.org.readthedocs.build/en/2208/api/generated/pygmt.Figure.timestamp.html#pygmt.Figure.timestamp

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

pygmt/src/timestamp.py Outdated Show resolved Hide resolved
@seisman seisman added the feature Brand new feature label Nov 30, 2022
@seisman seisman added this to the 0.8.0 milestone Nov 30, 2022
@seisman seisman changed the title WIP: Add Figure.timestamp to plot the GMT timestamp logo Add Figure.timestamp to plot the GMT timestamp logo Dec 2, 2022
Comment on lines 56 to 57
if is_nonstr_iter(offset): # given a list
kwdict["U"] += "+o" + "/".join(f"{item}" for item in offset)
Copy link
Member Author

Choose a reason for hiding this comment

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

Please note that we can't use decorator @kwargs_to_strings(offset="sequence") here, because the decorator can only deal with keyword arguments and doesn't work for the default values. I.e., it doesn't work for Figure.timestamp() but works for Figure.timestamp(offset=["-54p", "-54p"]), although offset defaults to ["-54p", "-54p"].

Copy link
Member Author

Choose a reason for hiding this comment

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

Related issue report at #2361.

@seisman seisman marked this pull request as ready for review December 2, 2022 15:44
@seisman seisman modified the milestones: 0.8.0, 0.9.0 Dec 20, 2022
@seisman seisman marked this pull request as draft December 30, 2022 07:59
@seisman
Copy link
Member Author

seisman commented Jan 2, 2023

Ping @GenericMappingTools/pygmt-maintainers for comments on the current implementation of the Figure.timestamp() function and ideas about how to test it.

@seisman
Copy link
Member Author

seisman commented Jan 3, 2023

The libfaketime package can be used to temporarily change the system date/time. It's also available on conda-forge, but it's a CLI program.

There are Python wrapper (https://github.com/simon-weber/python-libfaketime) and pytest plugin (https://github.com/touilleMan/pytest-libfaketime), but both are not actively maintained.

lib.call_module(
module="plot",
args=build_arg_string(kwdict)
+ f" --FONT_LOGO={font} --FORMAT_TIME_STAMP={timefmt}",
Copy link
Member Author

@seisman seisman Jan 3, 2023

Choose a reason for hiding this comment

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

Two thoughts when implementing this function:

  1. Add a new parameter (e.g., confdict) to the build_arg_string function, so that we can pass a dict of GMT defaults to the call_module function, e.g.,

    build_arg_string(kwdict, confdict={"FONT_LOGO": font, "FORMAT_TIME_STAMP": timefmt}
    
  2. The three parameters FONT_LOGO, MAP_LOGO, and MAP_LOGO_POS won't be used after the Figure.timestamp function is used (FORMAT_TIME_STAMP is still used in other places), so they can be removed from the pygmt.config keyword list:

    _keywords = [
    "COLOR_BACKGROUND",
    "COLOR_FOREGROUND",

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, point 1 seems ok, but I'm not sure about point 2. I'd prefer to keep the three parameters in the list, because even if you remove it, you're only removing the autocompletion, but users can still manually set those parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Point 1 was implemented in PR #2324

@weiji14
Copy link
Member

weiji14 commented Jan 3, 2023

7. It's difficult to write tests for this module, because the image has the timestamp string, which changes every second. We can't test it unless we can find a way to set the current time to a fixed value.

Maybe use @check_figures_equal following https://github.com/GenericMappingTools/pygmt/blob/v0.8.0/doc/contributing.md#using-check_figures_equal, and output only the date and not the precise time down to the second?

@seisman
Copy link
Member Author

seisman commented Jan 4, 2023

timefmt : str
    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).

I just realized that it's possible to specify a fixed timestamp by passing a text to the timefmt parameter (actually GMT's FORMAT_TIME_STAMP parameter).

Here is a CLI example:

gmt plot -T -U --FORMAT_TIME_STAMP="1970-01-01T00:00:00" -png map

map

I think we can trust the timestamp string is always correct and focus on testing the function parameters.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2023

Summary of changed images

This is an auto-generated report of images that have changed on the DVC remote

Status Path
added pygmt/tests/baseline/test_timestamp.png
added pygmt/tests/baseline/test_timestamp_font.png
added pygmt/tests/baseline/test_timestamp_justification.png
added pygmt/tests/baseline/test_timestamp_label.png
added pygmt/tests/baseline/test_timestamp_offset.png

Image diff(s)

Added images

  • pygmt/tests/baseline/test_timestamp.png

  • pygmt/tests/baseline/test_timestamp_font.png

  • pygmt/tests/baseline/test_timestamp_justification.png

  • pygmt/tests/baseline/test_timestamp_label.png

  • pygmt/tests/baseline/test_timestamp_offset.png

Modified images

Path Old New

Report last updated at commit 0079643

pygmt/src/timestamp.py Outdated Show resolved Hide resolved
pygmt/tests/test_timestamp.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Mar 2, 2023

Ping @GenericMappingTools/pygmt-maintainers for reviews.

Comment on lines +39 to +50
def test_timestamp_justification():
"""
Check if the "justification" parameter works.

Only a subset of justification codes are tested to avoid overlapping
timestamps.
"""
fig = Figure()
fig.basemap(projection="X10c/5c", region=[0, 10, 0, 5], frame=0)
for just in ["BL", "BR", "TL", "TR"]:
fig.timestamp(justification=just, timefmt=just)
return fig
Copy link
Member

Choose a reason for hiding this comment

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

The output of the justification image doesn't look correct?

All of the timestamps are on the bottom left (BL). Shouldn't they be at the four different corners?

Copy link
Member Author

@seisman seisman Mar 2, 2023

Choose a reason for hiding this comment

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

Unlike other embellishments (e.g., inset, legend or direction rose), the timestamp logo is always located at the lower left corner of the map. I believe this is GMT's design.

See https://docs.generic-mapping-tools.org/dev/gmt.conf.html#term-MAP_LOGO_POS

Sets the justification and the position of the logo/timestamp box relative to the current plot’s lower left corner (i.e., map origin)

Copy link
Member

Choose a reason for hiding this comment

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

Interesting design choice 😅 Actually, I just realized I was thinking of position rather than justification. Maybe we should document the default offset="-54p/-54p" under the offset docstring?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default values are clear from the function definition:

Figure.timestamp((text=None, label=None, justification='BL', offset=('-54p', '-54p'), font='Helvetica,black', timefmt='%Y %b %d %H:%M:%S')

I'm hesitate to document the defaults in the docstring.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, was thinking of adding [Default is ('-54p', '-54p')], but it's not really needed I guess.

pygmt/src/timestamp.py Outdated Show resolved Hide resolved
Comment on lines 45 to 46
Font of the timestamp and the optional label. The parameter can't
change the font color for GMT<=6.4.0.
Copy link
Member

Choose a reason for hiding this comment

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

Should a GMTInvalidInput error be raised when users pass font as a parameter with GMT 6.3.0? Or is the error clear enough from GMT?

Copy link
Member Author

Choose a reason for hiding this comment

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

The font parameter works for any GMT versions.

For GMT>6.4.0, it can change both the font ID and font color, but for GMT<=6.4.0, the font color is always black so it can only change the font ID.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, maybe reword it into something like this?

Suggested change
Font of the timestamp and the optional label. The parameter can't
change the font color for GMT<=6.4.0.
Font of the timestamp and the optional label. The parameter can't
change the font color for GMT<=6.4.0, only the font label.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid "font label" is incorrect here. It should be "font ID" or "font name".

Copy link
Member

Choose a reason for hiding this comment

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

Owh, you mean like font style (e.g. Helvetica)? Feel free to edit the suggested changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added "only the font ID" in 0079643.

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.

Only a very minor suggestion as mentioned in https://github.com/GenericMappingTools/pygmt/pull/2208/files#r1122590265 (up to you to apply), but otherwise wait for second reviewer to approve.

@weiji14 weiji14 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 2, 2023
pygmt/src/timestamp.py Outdated Show resolved Hide resolved
@michaelgrund
Copy link
Member

Would you also like to add a gallery example or should we do this in a separate PR @seisman ?

@seisman
Copy link
Member Author

seisman commented Mar 2, 2023

Would you also like to add a gallery example or should we do this in a separate PR @seisman ?

It's not on my todo list, so better to wait for someone else to work on the gallery example in another PR.

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