Skip to content
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

Add a --like option to rio-edit-info #399

Merged
merged 6 commits into from Jul 9, 2015
Merged

Conversation

sgillies
Copy link
Member

@sgillies sgillies commented Jul 2, 2015

In combination with, eg, --crs ?, this will extract the metadata value from another file.

So far, it seems like a value will be required for the options. I think ? makes a decent placeholder.

This PR also moves validation of the --crs, --transform, and --nodata options to callbacks, simplifying the command's function quite a bit.

Thoughts, @brendan-ward @jacquestardie @geowurster?

See #387.

In combination with, eg, `--crs ?`, this will extract the metadata
value from another file.

See #387.
@sgillies sgillies added this to the 0.25 milestone Jul 2, 2015
@geowurster
Copy link
Contributor

@sgillies It would be nice if None could be used to unset the nodata value. Currently this can only be done with gdalwarp/translate and -a_nodata/-dstnodata none. It looks like this requires some work in the Cython layer so unless this is a basic change, I'll open a ticket.

  File "rasterio/_io.pyx", line 1458, in rasterio._io.RasterUpdater.set_nodatavals (rasterio/_io.c:19834)
    nodataval = val
TypeError: a float is required

#387 suggests that the command below should only copy the CRS from the --like file and leave the nodata (and other vals untouched), but it seems to blindly apply everything. The added callbacks always pull from ctx.obj['like'] if it is available so I think thats part of the problem, unless I'm not understanding the intention.

$ rio edit-info --like EPSG4326-nodata100.tif --crs ? tests/data/RGB.byte.tif

This demonstrates the problem:

import rasterio as rio
from click.testing import CliRunner
from rasterio.rio.main import main_group


# This is the file to alter
basefile = 'BASE.tif'
meta = {
    'driver': 'GTiff',
    'height': 120,
    'width': 220,
    'nodata': 0.0,
    'dtype': rio.ubyte,
    'crs': 'EPSG:32610',
    'count': 1
}
with rio.open(basefile, 'w', **meta) as src:
    assert src.nodata == 0
    assert src.crs == {'init': 'epsg:32610'}


# This is the file to inherit from
template = 'LIKE.tif'
meta = {
    'driver': 'GTiff',
    'width': 100,
    'height': 200,
    'crs': 'EPSG:4326',
    'nodata': 100,
    'count': 1,
    'dtype': rio.ubyte
}
with rio.open(template, 'w', **meta) as src:
    assert src.nodata == 100
    assert src.crs == {'init': 'epsg:4326'}


result = CliRunner().invoke(main_group, [
    'edit-info',
    '--like', template,
    '--crs', '?',
    basefile
])
assert result.exit_code == 0


with rio.open(basefile) as src:
    assert src.crs == {'init': 'epsg:4326'}, 'CRS failed'
    assert src.nodata == 0, "nodata failed"

@sgillies
Copy link
Member Author

sgillies commented Jul 3, 2015

@geowurster thanks, I'll look into that.

gdal_translate -a_nodata none actually copies data to unset the nodata value :( There isn't a method in GDAL to remove nodata values until 2.1: http://trac.osgeo.org/gdal/wiki/rfc58_removing_dataset_nodata_value.

@geowurster
Copy link
Contributor

Oh right, duh.

combination of the --like, --crs, --nodata, --transform, and --all
options.

rio-edit-info example.tif --all --like template.tif
Copy link
Contributor

Choose a reason for hiding this comment

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

@sgillies typo: rio-edit-info -> rio edit-info

Also, the description above would be more clear like this:

    Metadata items may also be read from an existing dataset using a
    combination of the --like option with at least one of --all, --crs, 
    --nodata, and --transform options.

It is unclear what values are going to be copied from template when --like is provided but others are not. When I tested this option only, I got several unexpected results given that --all is supposed to be False by default:

$ cp tests/data/shade.tif /tmp/test.tif

$ rio info /tmp/test.tif
{"count": 1, "crs": "EPSG:3857", "res": [9.5546285343, 9.5546285343], "bounds": [-11858134.818413004, 4803914.3530251775, -11848350.87879388, 4813698.2926443005], "dtype": "uint8", "driver": "GTiff", "transform": [9.5546285343, 0.0, -11858134.818413004, 0.0, -9.5546285343, 4813698.2926443005], "lnglat": [-106.47949217280885, 39.605688173875606], "height": 1024, "width": 1024, "shape": [1024, 1024], "blockxsize": 1024, "tiled": false, "blockysize": 8, "nodata": 255.0}

$ rio edit-info /tmp/test.tif --like tests/data/RGB.byte.tif

$ rio info /tmp/test.tif
{"count": 1, "crs": "EPSG:32618", "res": [300.0379266750948, 300.041782729805], "bounds": [101985.0, 2519672.2144846795, 409223.8369152971, 2826915.0], "dtype": "uint8", "driver": "GTiff", "transform": [300.0379266750948, 0.0, 101985.0, 0.0, -300.041782729805, 2826915.0], "lnglat": [-77.40522561315726, 24.153251565772365], "height": 1024, "width": 1024, "shape": [1024, 1024], "blockxsize": 1024, "tiled": false, "blockysize": 8, "nodata": 255.0}

Looks like crs and transform were copied across but not nodata.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider updating the docstring comments for --transform because the value must be string quoted but isn't demonstrated that way:

--transform '[300.038, 0.0, 101985.0, 0.0, -300.042, 2826915.0]'

I don't know that calling them JSON-encoded makes that super clear in this context; they are just plain arrays, in quotes...

@brendan-ward
Copy link
Contributor

zsh doesn't appear to like the question marks used for placeholders:

$ rio edit-info /tmp/test.tif --like tests/data/RGB.byte.tif --nodata ?
zsh: no matches found: ?

Quoting like this fixes it '?' but is a bit awkward for a placeholder. Note: looks like - works fine as a placeholder too and causes no issues with zsh.

I'm finding the same issues as above related to more values than specified via the option getting copied across: --like <template> --nodata - also copies crs and transform even though it should copy nodata value.

@geowurster
Copy link
Contributor

Rather than use a single semi-ambiguous character, why not just use the word like or copy or something similar that doesn't conflict with a CRS definition? like matches the flag name and is pretty explicit.

The value is already being handled in a callback so we can make it whatever we want so we may as well take advantage of that and be cleaner and explicit. - typically means stdin/stdout and the more I think about ? the more it seems kind of ambiguous like ¯\_(ツ)_/¯ this value is going to come from somewhere ....

$ rio edit-info \
    Input.tif \
    --nodata 0 \
    --like Template.tif \
    --crs like \
    --transform like

Also add more tests of bad parameters.
@sgillies
Copy link
Member Author

sgillies commented Jul 7, 2015

Great comments, @brendan-ward @geowurster. I believe I've addressed them in the recent commit.

retval = json.loads(value)
except ValueError:
retval = value
if not (rasterio.crs.is_geographic_crs(retval) or
Copy link
Contributor

Choose a reason for hiding this comment

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

@sgillies consider adding a rasterio.crs.is_valid_crs function to handle this objective (internally it can make these two calls). It seems a little out of place to use these tests because in this context we don't care if it is geographic or projected, we only care that it is valid...

Sean Gillies added 2 commits July 8, 2015 07:50
Also add another test of the --all callback and fix the bug it
exposed.
brendan-ward added a commit that referenced this pull request Jul 9, 2015
Add a --like option to rio-edit-info
@brendan-ward brendan-ward merged commit 4e0769f into master Jul 9, 2015
@brendan-ward brendan-ward deleted the rio-edit-info-like branch July 9, 2015 04:17
@sgillies
Copy link
Member Author

sgillies commented Jul 9, 2015

Seems like this feature deserves a Ralph Wiggums gif.

@geowurster
Copy link
Contributor

People say the best parts of rasterio are that "its more Pythonic" or, "it has a modern and well composed CLI" or "it gives you a numpy array like, wicked fast", but they're all wrong. The best part of rasterio is topical Ralph Wiggum GIFs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants