-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
r.proj: Add test cases #1663
r.proj: Add test cases #1663
Conversation
aaronsms
commented
Jun 19, 2021
•
edited
Loading
edited
- Add test cases for r.proj
test_different_resolution: Test output with region using a different resolution. | ||
""" | ||
|
||
location = "nc_spm_08_grass7" |
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.
Currently, r.proj requires a location parameter, and although the workflow uses nc_spm dataset, they have different names in different context. Changing the hardcoded location to "nc_spm_full_v2alpha2" would probably solve the build issues. But is there a better way to handle this?
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.
This is where the testing framework design doesn't work that well. Another location with data is needed, but this test uses the NC SPM as another, so I don't know, perhaps it assumes it is not in NC SPM which is not true when the tests are automatically executed.
Maybe it is good enough test if the same location is used as both current and the other (I'm not sure how r.proj behaves in that case, e.g., if it tries to skip some the steps and just copy). If this is the case, getting the current location name would solve the naming issue.
The more advanced solution is to create a new location (e.g. NC SPF - EPSG:2264) and make it the current one. That's likely better done in a separate environment (copy of os.environ). See, e.g., code in r.import
or v.import
or the create_environment function doc. You will need to leave out the use_temp_region
part as it currently does not support passing of the env
parameter.
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.
Thanks! The pointer to the doc for creating new location is really useful. The current behavior of r.proj should perform the same computation (so it's not a direct copy) but show a warning that we are using the same location. Since for now it's more important for me to check the integrity of computation after applying parallelization, I will leave doing the advanced step for the future.
test_different_resolution: Test output with region using a different resolution. | ||
""" | ||
|
||
location = "nc_spm_08_grass7" |
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.
This is where the testing framework design doesn't work that well. Another location with data is needed, but this test uses the NC SPM as another, so I don't know, perhaps it assumes it is not in NC SPM which is not true when the tests are automatically executed.
Maybe it is good enough test if the same location is used as both current and the other (I'm not sure how r.proj behaves in that case, e.g., if it tries to skip some the steps and just copy). If this is the case, getting the current location name would solve the naming issue.
The more advanced solution is to create a new location (e.g. NC SPF - EPSG:2264) and make it the current one. That's likely better done in a separate environment (copy of os.environ). See, e.g., code in r.import
or v.import
or the create_environment function doc. You will need to leave out the use_temp_region
part as it currently does not support passing of the env
parameter.
self.runModule( | ||
"r.proj", | ||
input="elevation", | ||
location=self.location, |
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.
I think you will need to add mapset="PERMANENT"
.
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.
Noted!
for method, reference in self.references["same"].items(): | ||
output_file = "elevation_same_{}".format(method) | ||
self.to_clear.append(output_file) | ||
self.runModule( |
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.
assertModule
is better in this context as this tells the framework that this is the module you are testing. It may improve the error messages you (don't) get in the CI.
3d4ac9a
to
1d2e97b
Compare
18.04 uses PROJ 4 variant, and 20.04 uses PROJ 5+. r.proj has different behavior for each of them, perhaps some small computation discrepancies (thus the failing test in 18.04). |
(re-run flake8) |
1d2e97b
to
9a05d11
Compare
"r.proj", | ||
input="elevation", | ||
location=self.location, | ||
mapset=self.mapset, |
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.
So, shouldn't this be mapset="PERMANENT"
? (see comments above)