-
-
Notifications
You must be signed in to change notification settings - Fork 368
r.out.gdal: Skip nodata and range checks if export is forced #6170
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
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I see that r.out.gdal lacks a test. Will add one. [Done.] |
Another question regarding the introduction of pytest: |
Definitely yes, I just didn't make any steps towards that like deciding where to put them in the library, but grass.tests, grass.fixtures, or grass.test_fixtures seems like a good start. |
That's what the conftest.py files do. They apply to all subfolders when placed at one level. (So, there could be one in the vector/ folder, in the vector/v.Xxxx.yy folder etc). We just can't have one in the root of the repo, as autoconf overwrites or deletes them (they delete conf* files during configure). Both autotools and pytest won't budge on their positions. Since we don't have a src/ folder, we can't have a conftest file for pytest tests for the whole repo. |
This is becoming offtopic here, but we also have grass-addons repo where the folder-level conftest.py files from the main repo would not work. Anyway, we can definitely add a file to |
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 like this in general.
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.
Sorry for missing this one earlier. I checked my "Awaiting review from you" list and this one didn't have "review requested" so it was not on the list.
At this point, I'm confused about the changes. Is the PR title and description still relevant or is this just about the tests and the verbosity/warnings?
The main point of the PR is (still) to allow skipping nodata and data type checks to speed up export when users have made deliberate choices. For larger imagery groups that can save quite some time.... |
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 for bringing this back to my attention. It looks great.
This PR follows up a post on Discourse:
https://discourse.osgeo.org/t/r-out-gdal-performance-with-known-data-type-and-range/148619
The code changes make r.out.gdal skip data range and nodata checks if the f-flag (force export) as well as the nodata, and data type option are given by the user. In that case the assumption is the user has made a deliberate choice about this parameters. Skipping checks cuts export time almost by half...
In a workflow I have to regularly export data to multi-band GeoTiff (COG actually) where I know the data range and the suitable GDAL data type across all bands to export in advance. Even though I use the -f flag and set the nodata option, a full data type and nodata check is performed on every band. To me that feels like unnecessary processing and seems inconsistent with the documentation of the -f flag.
Any feedback is welcome...