Remove deprecated features

[geowurster]

Closes #516
Closes #794

Remove deprecated features and API's. See docs/migrating-to-v1.rst for a general description.

[geowurster]

I'll finish this one up late tonight or tomorrow. I think the only feature remaining is affine vs. transform.

[geowurster]

@perrygeo @sgillies @brendan-ward This isn't quite finished but it's kinda big so some 👀 now wouldn't hurt. I need to add add a section to the migration guide about affine vs. transform, look at how they are handled in rasterio.open(), and look at how to handle InMemoryRaster.transform, which is a GDAL geotransform and is only used internally, but presents a naming inconsistency for us. I think the last one will be better handled in a different ticket.

[eseglem]

Is this different than with rasterio.Env()? Or are they equivalent. In #760 everything was set to just rasterio.Env() and wanted to make sure that is right.

[geowurster]

@eseglem Yes, you are correct - thanks for catching this.

[brendan-ward]

@geowurster this is looking good.

I do wonder if we shouldn't have a period of time where we still use guard_transform but have it raise exceptions rather than warnings. Since we are retaining the same name, but changing the behavior (prior deprecation warnings notwithstanding), we risk throwing obscure errors instead of catching easy migration mistakes.

In addition to InMemoryRaster.transform, we are passing other GDAL style transforms from the Python API into the Cython implementations; this seems inconsistent too. E.g., rasterize -> _rasterize. Maybe we should have these be Affine all the way to Cython, and deal w/ the conversion there. This would be fine to do in a separate issue; it is more of a consistency problem.

Per your question above, I think raising a ValueError if we get a GDAL style transform when we expect an Affine is preferable to an assertion.

[perrygeo]

@geowurster The migration guide is fantastic and will help all of us out tremendously. Links to issues is a nice touch 👌

I have written oodles of code that uses the affine and relies on it passed to open. I misread the deprecation plan and often write code like the following, relying on the soon-to-be-deprecated behavior of affine overriding transform.

with rasterio.open(input) as src:
    profile = src.profile
...
profile['affine'] = new_affine
...
with rasterio.open(output, 'w', **profile) as dst:
    dst.write(arr)

So pragmatically we need to keep affine as a kwarg for open for a while otherwise lots of stuff is going to break at work 😞

To the other issues, 👍 to all of @brendan-ward's suggestions

[brendan-ward]

Would it be heresy to suggest that we keep affine and drop transform altogether? That way, in some internals (e.g., within Cython), we can keep transform where it specifically refers only to a GDAL style transform... Certainly would cut down on ambiguity, and also avoid the case of having to handle a GDAL style transform passed in when we expect an Affine.

I imagine no matter what we do here, we're likely to break code.

@perrygeo maybe we shouldn't jump to 1.0 until we're ready to roll out a few breaking API changes? I have mixed feelings about waiting until after 1.0 to roll those out. But I'm definitely OK with a few more <1.0 releases while we work through API changes.

[geowurster]

@brendan-ward @perrygeo Incorporated your suggestions and merged master again. Ready for another round.

[brendan-ward]

@geowurster Awesome! Changes look good to me and I think this is good to go.

Pending review from @perrygeo , I think we're ready to 🚀

Thank you for all the hard work on this!

[geowurster]

Added #794 to this PR.

[perrygeo]

@geowurster @sgillies I'm going to take a stab at resolving the merge conflicts and merging with master post-public-io. I'll push a commit shortly...

[perrygeo]

So it's all merged up with the public io work.

Testing this against some other rasterio scripts/modules, I've come across one instance where the affine→transform switch is going to break code:

This is a common pattern in some rasterio code, ironically intended as a way to prepare for the future by ensuring Affine objects get passed as the transform kwarg.

    with rasterio.open(src_path) as src:
        opts = src.profile.copy()

    opts['transform'] = opts['affine']

Which now gives you KeyError: 'affine'

A couple of ways to proceed, there may be other options.

• backwards compatibility on a major version is not guaranteed so just keep it as is. Recommend downstream projects just remove that single line, it would still work with a warning on 0.x and work perfectly on 1.x.
• continue putting affine into the profile dictionary for a period of time or indefinitely.

Other than that potential backwards incompatibility, this looks great. @sgillies @geowurster , thoughts?

[geowurster]

@perrygeo Nice! We're keeping src.affine, so leaving the 'affine' key would be, and it would make the transition smoother, but it would be great if we could alert users to its presence. We could issue a warning if src.profile / src.meta returned something like the object below.
Subclass dict directly since we just need to intercept some methods, and so we don't have surprises like Toblerity/Fiona#367.

class _TempProfile(dict):

    def __init__(self, *args, **kwargs):
        super(dict, self).__init__(*args, **kwargs)
        if 'affine' in self:
            with warnings.catch_warnings():
                warnings.simplefilter('always')
                warnings.warn("The 'affine' key in src.profile and src.meta will go away eventually", DeprecationWarning)

    def __getitem__(self, item):
        if item == 'affine':
            with warnings.catch_warnings():
                warnings.simplefilter('always')
                warnings.warn("The 'affine' key in src.profile and src.meta will go away eventually", DeprecationWarning)
        return super(dict, self).__getitem__(item)

    def __setitem__(self, item):
        # Same as __getitem__

    def copy(self):
        # This would spit out another warning from __init__, but being extra noisy isn't necessarily bad.
        # We could disable it with an argument in init like _initial_check=False
        return _TempProfile(super(dict, self).copy())

[perrygeo]

@geowurster I like that direction. We could even shadow affine, effectively making it an alias for transform on get/set so that the dictionary never contains affine

class _TempProfile(dict):
    def __init__(self, *args, **kwargs):
        super(_TempProfile, self).__init__(*args, **kwargs)
        if 'affine' in self:
            with warnings.catch_warnings():
                warnings.simplefilter('always')
                warnings.warn("The 'affine' key in src.profile and src.meta "
                              "will go away eventually. Use 'transform' "
                              "instead.", DeprecationWarning)
            del self['affine']

    def __getitem__(self, item):
        if item == 'affine':
            with warnings.catch_warnings():
                warnings.simplefilter('always')
                warnings.warn("The 'affine' key in src.profile and src.meta "
                              "will go away eventually. Using 'transform' "
                              "instead.", DeprecationWarning)
            return super(_TempProfile, self).__getitem__('transform')
        return super(_TempProfile, self).__getitem__(item)

    def __setitem__(self, item, val):
        if item == 'affine':
            with warnings.catch_warnings():
                warnings.simplefilter('always')
                warnings.warn("The 'affine' key in src.profile and src.meta "
                              "will go away eventually. Using 'transform' "
                              "instead.", DeprecationWarning)
            return super(_TempProfile, self).__setitem__('transform', val)
        return super(_TempProfile, self).__setitem__(item, val)

[geowurster]

@perrygeo Yeah, but we don't know what the user is doing with the dictionary, so that could create some really wacky behavior if it is used outside of the Rasterio API.

[perrygeo]

The reason the alias approach might be nice is that warning would only be triggered if a user was explicitly accessing those keys. In the absence of using an affine key, this should behave as a normal dictionary. Granted it's a bit of an unconventional approach so I may not have anticipated all the potential issues.

In the other approach, every user would see multiple warnings every time they used a profile, regardless of whether they get/set affine. IOW our own code would be triggering warnings when a user initialized the profile and when they copied or destructured it.

... As an aside, it might be fun to build an AliasDict data structure that could behave like this more generally so that e.g. Thom, Tom and Thomas could all be used as keys but stored as a single key-value pair while optionally warning that the correct name is Thomas. Sorry for the complete tangent, now back to your regularly scheduled programming...

[geowurster]

Ah I understand what you are saying now. We are providing an object to the user but it contains something we don't really want them to have, so we don't give it to them unless they know to ask for it. The fact that it is a dictionary is somewhat irrelevant because the users want the content, so really its just behaving like a dictionary in a weird meta way. Aliasing the key does make a certain amount of sense, and I'm guessing users are only referencing profile outside of rasterio.open() like array.astype(profile['dtype']), so this might actually work ...

What if we just disguised this as a new RasterProfile() similar to CRS()? We could make it "private" so users aren't tempted to construct it themselves, and we could revert to a dict in a later version. We make the docs for both src.profile and RasterProfile() super clear about its purpose and insist any isinstance(src.profile) checks compare against dict and not RasterProfile(). I like sticking to Python's builtin types when possible, and there isn't a good reason for the profile to be an object, so we should resist the temptation to add methods and properties.

Now that I think about it again you're right - issuing a warning in __init__ isn't necessary. We could have a flag to disable when we construct, but if the user isn't constructing the object it doesn't matter.

... Optimizing AliasDict() would be the extra fun part. As an extra aside, you may like sortedcontainers. It needs a SortedDefaultDict() but its pretty cool as is...

[perrygeo]

I like sticking to Python's builtin types when possible, and there isn't a good reason for the profile to be an object, so we should resist the temptation to add methods and properties.

👍

My thinking is that src.profile and src.meta would just return a _RasterProfile and it would behave just like a normal dict aside from the affine deprecation warning. When the time comes to end affine deprecation and completely remove it, we would revert back to a standard dict.

FWIW, I worked up a general AliasDict implementation here: https://gist.github.com/perrygeo/f79b2ea8f98d4f01898bd5e0c6ce8344 . Not suggesting we use it here, just for fun.

@sgillies the two remaining decision points here are

• Do we try to keep affine in the profile dict for the deprecation period? Or do we break backwards compatibility?
• Assuming the former, how do we implement that? Just putting affine in the standard profile dict would lead to unavoidable warnings when destructuring open(..., **profile). A dict subclass which aliases affine to transform might work but is a bit unconventional.

[perrygeo]

The _RasterProfile solution can be reviewed in the drop-dep-rasterprofile branch: drop-dep...drop-dep-rasterprofile

[geowurster]

@perrygeo Will try to review later today but may not have time.

[sgillies]

I'd prefer we simply warn and not mess with catching or filtering and encourage developers to test their programs with python -W "error::DeprecationWarning:rasterio".

[perrygeo]

What about FutureWarning which seems to be a bit louder by default than DeprecationWarning.

[sgillies]

I like the profile object idea. I think the essence of it is that it should warn (DeprecationWarning) if you access its affine item and should raise AttributeError if you try to set its affine item.

I regret that we didn't already deprecate the affine kwarg for open(). My mistake there.

We already have a profiles module and profile classes. Let's use these as a jumping off point.

[perrygeo]

@sgillies I forgot all about the profile class, mainly because it's only used to construct the rasterio.profiles.default_gtiff_profile dictionary and instances are not currently returned by the dataset's profile or meta property. Moving the logic there and having DatasetReader.profile return an instance of rasterio.profiles.Profile sounds +1

Warning on __getitem__, AttributeError on __setitem__ sounds like a solid approach for dealing with affine in that class.

[perrygeo]

Looking at rasterio.profiles design more closely, I'm not exactly sure how to proceed.

The Profile class is just a wrapper class with a __call__ method - not sure what advantages this
provides over a function. And since it doesn't inherit from dict, I don't see an easy way to merge the intent of this design with the new requirements for deprecating a key.

[perrygeo]

@geowurster @sgillies Let's merge this and deal with the affine key regression in #823

[dmwelch]

Just my 2 cents, but transform should be preferred over affine since all affines are transforms but not all transforms are affine.

A use case (I'm used to medical images, so bear with me) could be non-linear registration to improve geolocation accuracy by correcting for atmospheric distortion using control points or automated feature extraction. That's not currently supported, but it would be a powerful addition. I think ORFEO has some of that functionality???

[geowurster]

@dmwelch Do you mean referring to the geospatial transformation construct as transform instead of affine? If so, that is one of the changes handled by this PR in addition to some docs that explain the transform vs. affine issue and its history in detail.

There's a lot going on in this thread, but the more recent parts about the affine property and key are more about how to deprecate them without breaking existing code. The issue is pointed out in #763 (comment). The short story is that can't just remove src.profile['affine'] because a really common pattern for making sure you get an affine.Affine() is to do profile['transform'] = profile['affine'], which would raise KeyError's for a lot of users.