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

GeoTiff Companion Object Methods #1757

Closed
echeipesh opened this issue Oct 31, 2016 · 6 comments
Closed

GeoTiff Companion Object Methods #1757

echeipesh opened this issue Oct 31, 2016 · 6 comments
Milestone

Comments

@echeipesh
Copy link
Contributor

A common pattern in scala to use companion object to provide constructor overloads for the base class. This is also convenient place to provide read constructors and in general decoders. An example can be seen here: https://github.com/geotrellis/geotrellis/blob/master/raster/src/main/scala/geotrellis/raster/io/geotiff/SinglebandGeoTiff.scala#L45

This API design/improvement issue.

In this particular case SinglebandGeoTiff is part of a type hierarchy with MultibandGeoTiff where they both extend GeoTiff interface. Because this distinction is forced by type structure of GeoTrellis and not really a distinction that users have in mind when working with GeoTiff files we would like to extend the companion overload pattern to the GeoTiff super-type. That is:

When working with GeoTiff (either singleband or multiband) there should be an apply on GeoTiff companion object that converts the arguments to either SinglebandGeoTiff or MultibandGeoTiff. When types are not enough to specify what is intended a method will need to have a unique name, such as for reading.

For instance a String can be converted to a GeoTiff by reading the file:

GeoTiff.readSingleband(path: String): SinglebandGeoTiff has the same parameter signature as GeoTiff.readMultiband(path: String): MultibandGeoTiff, so therefore needs the name.

Also ProjectedRaster[T] has enough information to be converted to a either a SinglebandGeoTiff or MultibandGeotiff. But in this case can be handled with an apply overload.

@echeipesh
Copy link
Contributor Author

I am looking at our documentation and it's a little sparse on this topic: https://github.com/geotrellis/geotrellis/blob/master/docs/raster/Geotiffs.md

As part of this Issue there should be an addition to the docs that talks about and provides sample code:

This doesn't need be in-depth, just enough to give somebody an example and a flavor.

@echeipesh echeipesh added this to the 1.0 milestone Oct 31, 2016
@jbouffard
Copy link
Contributor

I'm actually in the process of expanding/writing docs on some of the topics you suggested.

Example: Reading GeoTiffs Locally

@echeipesh
Copy link
Contributor Author

@jbouffard Those are good docs. Since you were looking at his part of the API whats your opinion on the issue, does it seems like centralizing some common overloads around GeoTiff would make it cleaner?

@jbouffard
Copy link
Contributor

@echeipesh Thanks. Yeah, I think that centralizing the common overloads to a GeoTiff companion object would make things it easier for people to use and read. I was surprised there wasn't one when I was first using GeoTrellis. We might want to consider keeping the old overloads for both SinglebandGeoTiffs and MultibandGeotiffs, though. As a lot of previous code uses them.

@echeipesh
Copy link
Contributor Author

+1 to keeping the old overloads

@lossyrob
Copy link
Member

Fixed by #1840

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

No branches or pull requests

3 participants