-
Notifications
You must be signed in to change notification settings - Fork 220
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
Initialize data version control for managing test images #1036
Merged
Merged
Changes from 10 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
48fb2d9
Initialize data version control
weiji14 4999875
Set dvc remote as https://dagshub.com/GenericMappingTools/pygmt.dvc
weiji14 9b61c77
Temporarily installing dvc using pip instead of conda to make CI work
weiji14 0c35dff
Refactor test_logo to use mpl_image_compare and track png files in dvc
weiji14 567c967
Add dvc install and dvc pull as a step in ci_tests.yaml to pull in data
weiji14 7e0940c
Merge branch 'master' into data_version_control
weiji14 4833466
List files in pygmt directory to see what happens after dvc pull
weiji14 f0ab167
Do `dvc pull` before `pip install` otherwise test PNGs aren't there
weiji14 f5e25fe
Merge branch 'master' into data_version_control
weiji14 6bd7ba9
First draft of instructions for using dvc to store baseline images
weiji14 e30c708
Instruct to do `git push` first and then `dvc push`
weiji14 df1ab56
Merge branch 'master' into data_version_control
weiji14 3208519
New checklist item for maintainers to get added to DAGsHub dvc remote
weiji14 2bd88c8
Move pygmt/tests/baseline/.gitignore to top-level
weiji14 93f6d6e
Just use `dvc push` without setting --remote upstream
weiji14 1f06f9a
Clarify that `git rm -r --cached` only needs to run during migration
weiji14 e36fd28
Try installing dvc from conda again now that there is a Py3.9 package
weiji14 f34bb09
Merge branch 'master' into data_version_control
weiji14 f3aa3c5
Install dvc and do `dvc pull` on GMT dev tests too
weiji14 af79eef
Refactor test_logo tests to be simpler and more unit-test like
weiji14 5860a72
Mention dvc status command to see which files need staging
weiji14 c37bdff
Use images for logo created using GMT 6.1.1
weiji14 393773b
List only files under pygmt/tests/baseline
weiji14 14cabd7
Update test_image to use SI units and long aliases
weiji14 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
/config.local | ||
/tmp | ||
/cache |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
[core] | ||
remote = upstream | ||
['remote "upstream"'] | ||
url = https://dagshub.com/GenericMappingTools/pygmt.dvc |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
# Add patterns of files dvc should ignore, which could improve | ||
# the performance. Learn more at | ||
# https://dvc.org/doc/user-guide/dvcignore |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
/test_*.png | ||
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
|
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
outs: | ||
- md5: 905d5b9f0f8d8b809899dfe9e87d0e91 | ||
size: 33347 | ||
path: test_logo.png |
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
outs: | ||
- md5: 25854f87e4e6f4c30be71bc08a3d430c | ||
size: 114522 | ||
path: test_logo_on_a_map.png |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,26 @@ | ||
""" | ||
Tests for fig.logo. | ||
""" | ||
import pytest | ||
from pygmt import Figure | ||
from pygmt.helpers.testing import check_figures_equal | ||
|
||
|
||
@check_figures_equal() | ||
@pytest.mark.mpl_image_compare | ||
def test_logo(): | ||
""" | ||
Plot a GMT logo of a 2 inch width as a stand-alone plot. | ||
""" | ||
fig_ref, fig_test = Figure(), Figure() | ||
# Use single-character arguments for the reference image | ||
fig_ref.logo(D="x0/0+w2i") | ||
fig_test.logo(position="x0/0+w2i") | ||
return fig_ref, fig_test | ||
fig = Figure() | ||
fig.logo(position="x0/0+w2i") | ||
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return fig | ||
|
||
|
||
@check_figures_equal() | ||
@pytest.mark.mpl_image_compare | ||
def test_logo_on_a_map(): | ||
""" | ||
Plot a GMT logo in the upper right corner of a map. | ||
""" | ||
fig_ref, fig_test = Figure(), Figure() | ||
# Use single-character arguments for the reference image | ||
fig_ref.coast(R="-90/-70/0/20", J="M6i", G="chocolate", B="") | ||
fig_ref.logo(D="jTR+o0.1i/0.1i+w3i", F="") | ||
|
||
fig_test.coast( | ||
region=[-90, -70, 0, 20], projection="M6i", land="chocolate", frame=True | ||
) | ||
fig_test.logo(position="jTR+o0.1i/0.1i+w3i", box=True) | ||
return fig_ref, fig_test | ||
fig = Figure() | ||
fig.coast(region=[-90, -70, 0, 20], projection="M6i", land="chocolate", frame=True) | ||
fig.logo(position="jTR+o0.1i/0.1i+w3i", box=True) | ||
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return fig |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I test this branch by adding a new baseline image, I got this error. I believe it won't be a issue after we migrate all baseline images to DAGsHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's why I added the optional
git rm -r --cached 'pygmt/tests/baseline/test_logo.png'
line in the CONTRIBUTING.md docs. We can slowly do the migration of images to DAGsHub later, a bit like so:@check_figures_equal()
to@pytest.mark.mpl_image_compare
@check_figures_equal
tests to@pytest.mark.mpl_image_compare
(can prioritize the slow tests as reported in Show test execution times in pytest #835/Improve some tests to speed up the CI #840)@check_figures_equal()
, removing it from codebase and documentation in CONTRIBUTING.md, also close Directly check if two figures returned by a function are equal matplotlib/pytest-mpl#95?@pytest.mark.mpl_image_compare
onlyI think we definitely need a bit more practice and train our contributors on this new
dvc
way of making test plots. If we like this new style (give it a few weeks/months), it might be worth considering telling upstream GMT on how to store all the postscript files (bit of a shame though with GenericMappingTools/gmt#3344) 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pygmt.test()
won't work for users.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, when I run
dvc add
, it gives me an error, then I need to rungit rm -r --cache
to remove the image from the git repo. After that, I need to rundvc add
again to make it work.I'm not sure how long the migration will take. If we expect it will take weeks or months, we need to improve the instructions.
When we migrate the old images to DAGsHub, we need to run:
when we add new baseline images, we need to run:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about swapping the order around so that
git rm -r --cached
is first?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
git rm
command will give an error if someone wants to add a new image (i.e., the image is not in the git repository). Perhaps change the comment# optional
to# only run this if migrating an existing image from git to dvc
? And remove it after we finish the migration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll change the comment to read that (but shorten it a bit).