Skip to content

Update failing doctests with upstream dataset changes - #2988

Merged
seisman merged 9 commits into
mainfrom
inline-doctests
Jan 15, 2024
Merged

Update failing doctests with upstream dataset changes#2988
seisman merged 9 commits into
mainfrom
inline-doctests

Conversation

@seisman

@seismanseisman commented Jan 11, 2024

Copy link
Copy Markdown
Member

Description of proposed changes

Address #2961 (comment).

Comment thread.github/workflows/ci_tests.yaml Outdated
@seisman

Copy link
Copy Markdown
MemberAuthor

There are still two failures:

===================================FAILURES===================================_______________ [doctest] pygmt.datasets.tile_map.load_tile_map________________105Raises106------107ImportError108If``contextily``isnotinstalledorcan'tbeimported. Follow109 :doc:`install instructions for contextily <contextily:index>`, (e.g.
110via``python-mpipinstallcontextily``) beforeusingthisfunction.
111112Examples113--------114>>>importcontextilyUNEXPECTEDEXCEPTION: ModuleNotFoundError("No module named 'contextily'")
Traceback (mostrecentcalllast):
File"/home/runner/micromamba/envs/pygmt/lib/python3.9/doctest.py", line1334, in__runexec(compile(example.source, filename, "single",
File"<doctest pygmt.datasets.tile_map.load_tile_map[0]>", line1, in<module>ModuleNotFoundError: Nomodulenamed'contextily'/home/runner/work/pygmt/pygmt/pygmt/datasets/tile_map.py:114: UnexpectedException__________________ [doctest] pygmt.src.grdproject.grdproject___________________179>>>importpygmt180>>># Load a grid of @earth_relief_30m data, with a longitude range of181>>># 10° E to 30° E, and a latitude range of 15° N to 25° N182>>>grid=pygmt.datasets.load_earth_relief(
183 ... resolution="30m", region=[10, 30, 15, 25]
184 ... )
185>>># Create a new grid from the input grid, set the projection to186>>># Mercator, and set inverse to "True" to change from "geographic"187>>># to "rectangular"188>>>new_grid=pygmt.grdproject(grid=grid, projection="M10c", inverse=True)
Error: TEDEXCEPTION: GMTCLibError("Module 'grdproject' failed with status code 72:\ngrdproject [ERROR]: Option -J: If map width is given you must also specify a region with -R")
Traceback (mostrecentcalllast):
File"/home/runner/micromamba/envs/pygmt/lib/python3.9/doctest.py", line1334, in__runexec(compile(example.source, filename, "single",
File"<doctest pygmt.src.grdproject.grdproject[2]>", line1, in<module>File"/home/runner/work/pygmt/pygmt/pygmt/helpers/decorators.py", line603, innew_modulereturnmodule_func(*args, **kwargs)
File"/home/runner/work/pygmt/pygmt/pygmt/helpers/decorators.py", line776, innew_modulereturnmodule_func(*bound.args, **bound.kwargs)
File"/home/runner/work/pygmt/pygmt/pygmt/src/grdproject.py", line122, ingrdprojectlib.call_module(
File"/home/runner/work/pygmt/pygmt/pygmt/clib/session.py", line624, incall_moduleraiseGMTCLibError(
pygmt.exceptions.GMTCLibError: Module'grdproject'failedwithstatuscode72:
Error: ect [ERROR]: Option-J: Ifmapwidthisgivenyoumustalsospecifyaregionwith-R/home/runner/work/pygmt/pygmt/pygmt/src/grdproject.py:188: UnexpectedException-----------------------------Capturedstderrcall-----------------------------Error: ect [ERROR]: Option-J: Ifmapwidthisgivenyoumustalsospecifyaregionwith-R

@seisman

seisman commented Jan 11, 2024

Copy link
Copy Markdown
MemberAuthor

The pygmt.datasets.tile_map.load_tile_map failure is because contextily is an optional dependency and is not installed in the Python 3.9 job run. It also means we can't run full tests in the Tests workflow, but fortunately we can still run them in GMT Legacy Tests and Dev Tests.

The pygmt.src.grdproject.grdproject is likely an upstream bug, reported in GenericMappingTools/gmt#8273.

@seismanseisman changed the title Fix failing inline doctestsUpdate failing doctests with upstream dataset changesJan 11, 2024
@seisman
seisman marked this pull request as ready for review January 11, 2024 14:15
@seismanseisman added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog labels Jan 11, 2024
@seismanseisman added this to the 0.11.0 milestone Jan 11, 2024
@seismanseisman added the needs review This PR has higher priority and needs review. label Jan 14, 2024
@weiji14

Copy link
Copy Markdown
Member

The pygmt.datasets.tile_map.load_tile_map failure is because contextily is an optional dependency and is not installed in the Python 3.9 job run. It also means we can't run full tests in the Tests workflow, but fortunately we can still run them in GMT Legacy Tests and Dev Tests.

This doctest should be skipped with the __doctest_requires__ = {("load_tile_map"): ["contextily"]} line (see https://github.com/scientific-python/pytest-doctestplus/tree/v1.1.0#doctest-dependencies and #2125 (comment)). I thought we had pytest-doctestplus installed here:

Is that doctest still being run somehow?

@seisman

Copy link
Copy Markdown
MemberAuthor

The pygmt.datasets.tile_map.load_tile_map failure is because contextily is an optional dependency and is not installed in the Python 3.9 job run. It also means we can't run full tests in the Tests workflow, but fortunately we can still run them in GMT Legacy Tests and Dev Tests.

This doctest should be skipped with the __doctest_requires__ = {("load_tile_map"): ["contextily"]} line (see https://github.com/scientific-python/pytest-doctestplus/tree/v1.1.0#doctest-dependencies and #2125 (comment)). I thought we had pytest-doctestplus installed here:

Is that doctest still being run somehow?

The doctest will be skipped only if the --doctest-plus option is given. So, the doctest is skipped for make test but will be run for make fulltest (see

test: PYTEST_ARGS=--doctest-plus $(PYTEST_COV_ARGS) ${PYTEST_EXTRA}
).

@weiji14

Copy link
Copy Markdown
Member

The doctest will be skipped only if the --doctest-plus option is given. So, the doctest is skipped for make test but will be run for make fulltest (see

test: PYTEST_ARGS=--doctest-plus $(PYTEST_COV_ARGS) ${PYTEST_EXTRA}

Hmm, you mentioned at #1790 (comment) that we were using --doctest-modules --doctest-plus for make fulltest, but looking at the code, it looks like only --doctest-modules is applied (from pyproject.toml).

It also means we can't run full tests in the Tests workflow, but fortunately we can still run them in GMT Legacy Tests and Dev Tests.

Do we need to run the full tests on GMT Legacy/Dev? The ci_doctests.yaml should cover the doctests, and ci_tests.yaml should cover the non-doctest tests already right?

@seisman

Copy link
Copy Markdown
MemberAuthor

The doctest will be skipped only if the --doctest-plus option is given. So, the doctest is skipped for make test but will be run for make fulltest (see

test: PYTEST_ARGS=--doctest-plus $(PYTEST_COV_ARGS) ${PYTEST_EXTRA}

Hmm, you mentioned at #1790 (comment) that we were using --doctest-modules --doctest-plus for make fulltest, but looking at the code, it looks like only --doctest-modules is applied (from pyproject.toml).

Hmm, I think the description in #1790 (comment) is incorrect. make test uses --doctest-modules --doctest-plus and make fulltest uses --doctest-modules. I've updated the comment.

It also means we can't run full tests in the Tests workflow, but fortunately we can still run them in GMT Legacy Tests and Dev Tests.

Do we need to run the full tests on GMT Legacy/Dev? The ci_doctests.yaml should cover the doctests, and ci_tests.yaml should cover the non-doctest tests already right?

You're right, I almost forgot this workflow. I was looking at the checklist at #2843, which only mentions "GMT Legacy" and "GMT Tests" but not "Doctests".

I think we should mention the "DocTests" workflow in both checklists (#2843 and #2961).

@weiji14

weiji14 commented Jan 15, 2024

Copy link
Copy Markdown
Member

It also means we can't run full tests in the Tests workflow, but fortunately we can still run them in GMT Legacy Tests and Dev Tests.

Do we need to run the full tests on GMT Legacy/Dev? The ci_doctests.yaml should cover the doctests, and ci_tests.yaml should cover the non-doctest tests already right?

You're right, I almost forgot this workflow. I was looking at the checklist at #2843, which only mentions "GMT Legacy" and "GMT Tests" but not "Doctests".

Yeah, could you trigger the "Doctests" workflow in this PR? I did try it locally and it seems to work fine without contextily if you add --doctest-plus here:

pygmt/Makefile

Lines 52 to 54 in 621d6da

# run doctests only
doctest: PYTEST_ARGS=--ignore=../pygmt/tests ${PYTEST_EXTRA}
doctest: _runtest

I think we should mention the "DocTests" workflow in both checklists (#2843 and #2961).

Sounds good.

@weiji14

weiji14 commented Jan 15, 2024

Copy link
Copy Markdown
Member

Yeah, could you trigger the "Doctests" workflow in this PR? I did try it locally and it seems to work fine without contextily if you add --doctest-plus here:

Owh, but using --doctest-plus only runs 34 tests, compared to 74 doctests if using only --doctest-modules. Maybe don't add that --doctest-plus line to make doctest then, as we agreed on in #1790 (comment).

>>> region = [10, 30, 15, 25]
>>> grid = pygmt.datasets.load_earth_relief(resolution="30m", region=region)
>>> # Project the geographic gridded data onto a rectangular grid
>>> new_grid = pygmt.grdproject(grid=grid, projection="M10c", region=region)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh wow, did we write the wrong description before. Default (when inverse=False) should have been "geographic" to "rectangular" 😅

Comment thread.github/workflows/ci_doctests.yaml Outdated
@seismanseisman 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 Jan 15, 2024
@seisman
seisman merged commit ec75e7b into mainJan 15, 2024
@seisman
seisman deleted the inline-doctests branch January 15, 2024 14:06
@seismanseisman removed the final review call This PR requires final review and approval from a second reviewer label Jan 15, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenanceBoring but important stuff for the core devsskip-changelogSkip adding Pull Request to changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@seisman@weiji14