MHS/DAS 1480/fixes regression test and debugging instructions #14
Conversation
flamingbear
force-pushed
the
mhs/DAS-1480/regression-test-and-debugging-instructions
branch
from
137bdb4
to
9396904
Compare
There was a problem hiding this comment.
The debugging instructions are incredibly useful and should give other developers a really useful tool during development. Thanks for adding them.
I mostly only had nitpicks, but before hitting approve, would be interested to know what you think about the any condition. It might just be that I've misunderstood what it's trying to do.
gdal_subsetter/transform.py
Outdated
| @@ -696,7 +696,8 @@ def recolor(self, layerid, srcfile, dstdir): | |||
| if relatedUrl.type == 'Color Map': | |||
| colormap = '/vsicurl/' + relatedUrl.url | |||
| discrete = True | |||
| if colormap is None and ('png' in fmt.mime or 'jpeg' in fmt.mime): | |||
| image_types = ['png', 'jpeg', 'tiff'] | |||
| if colormap is None and any(i_type for i_type in image_types if i_type in fmt.mime): | |||
There was a problem hiding this comment.
Could this any statement be simplified to:
if colormap is None and any(i_type in fmt.mime for i_type in image_types):
...Although this condition feels like it was originally a little off. I think what's trying to be checked here is whether the MIME type is a PNG, JPEG or (now) a TIFF. In which case, it might be better to have:
if colormap is None and fmt.mime in ['image/png', 'image/jpeg', 'image/tiff', 'image/tif']:
...Note that image/tiff and image/tif are both valid MIME types for a TIFF, so that should be included in whatever ends up happening. (I wonder also if it could/should be fmt.mime.lower(), to ensure everything is lowercase)
There was a problem hiding this comment.
Definitely like your list comprehension better than my original one. That's just me rusty in my pythons. Agree on other thoughts as well.
There was a problem hiding this comment.
Ugh, yeah, that was ugly what I was trying to do. bbf694d is correct version
DAS-1480
DAS-1480
DAS-1480
Originally recolor was called and every type of input was passed through gdaldem, but we discovered this is bad for geotiffs because they can be single band values and we don't want to split those into 4 color images. DAS-1480
DAS-1480
DAS-1480
| from harmony.message import Message | ||
|
|
||
|
|
||
| class TestRecolor(TestCase): |
There was a problem hiding this comment.
Thanks for adding some tests - these look great!
| if colormap is None and ('png' in fmt.mime or 'jpeg' in fmt.mime): | ||
|
|
||
| # Don't color tiffs | ||
| colorable_mime_types = ['image/png', 'image/jpeg'] |
There was a problem hiding this comment.
Even though this variable is only being used once, I like it because it makes it really clear what the next line is checking for. (Also it's easy to update the list without having to make the line below jump through formatting hoops for line length)
flamingbear
deleted the
mhs/DAS-1480/regression-test-and-debugging-instructions
branch
Description
Fixes regression tests for AVNIR related to previous release.
Jira Issue ID
DAS-1480
Local Test Steps
No logical tests unless you want to see if you can get the debugging running on your machine.
PR Acceptance Checklist
(this is a cleanup PR)
version.txtandCHANGE.mdupdated if any service code is changed.