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 interpretAs and withNoData methods to Tile #1702

Merged
merged 9 commits into from
Oct 30, 2016

Conversation

echeipesh
Copy link
Contributor

@echeipesh echeipesh commented Oct 25, 2016

Fixes: #1674

Allows for more flexibility when working around NoData values.

Additionally this expands the capacity of the ConstantTile to represent constant tiles other than tiles with ConstantNoDataHandling to satisfy the conversions implied by the new API.

TODO:

  • Unit tests for interpret
  • GeoTiffMultibandTile.interpretAs
  • Tile.withNoData
  • Tile concrete classes noDataValue constructor overload
  • New API review / discussion

@echeipesh echeipesh changed the title [WIP] Add interpret and asRawTile methods to Tile interface Add interpret and asRawTile methods to Tile interface Oct 25, 2016
@lossyrob
Copy link
Member

Is this WIP until the RFC gets worked out?

@echeipesh
Copy link
Contributor Author

I'd be happy to bundle them together, but they don't have to be. This PR can be merged as is and addresses the issue that spawned it.

@echeipesh echeipesh changed the title Add interpret and asRawTile methods to Tile interface [WIP] Add interpret and asRawTile methods to Tile interface Oct 26, 2016
@echeipesh echeipesh force-pushed the feature/eac/tile-interpret branch from b9c3a05 to 6f3a2ab Compare October 26, 2016 21:35
@echeipesh echeipesh changed the title [WIP] Add interpret and asRawTile methods to Tile interface Add interpret and asRawTile methods to Tile interface Oct 26, 2016
@echeipesh
Copy link
Contributor Author

@lossyrob @moradology Ready for review again after .withNoData changes.

One final through is that what is currently .interpretAs might be better named .withCellType. That name seems consistent with behavior:

  • If we're not changing underlying data type this just re-wraps the array with new CellType.
  • Otherwise we coerce to the desired cell type and label it with new NoDataHandling

@lossyrob
Copy link
Member

@echeipesh I think withCellType is a bit ambiguous, and might be read as doing the same thing convert does. But I've though about this less, so if you are strong +1 on that name I'll follow your lead.

@moradology moradology mentioned this pull request Oct 27, 2016
@moradology
Copy link
Contributor

moradology commented Oct 27, 2016

I also think withCellType seems more ambiguous. Have we considered, as an alternative to interpretAs, reinterpret? The other reason I like interpret is that it makes clear that we're dealing with something which quite likely modifies the Tile's semantics (making its numbers mean different things on the output than they meant prior to the method having been called).

@echeipesh
Copy link
Contributor Author

I prefer interpretAs to reinterpret. I thought that withCellType has nice symmetry to withNoData but lets discount it since its got two votes against it. I'm still not totally happy with interpretAs, I'd like to capture the intuition is that this is equivalent of calling _.toDouble or _.toInt on cell by cell basis.

  • toCellType(IntCells)
  • asCellType(IntCells)
  • convertRaw(IntCells)
  • convertFromRaw(IntCells)
  • reassign(IntCells)
  • coerce(IntCells)

@fosskers
Copy link
Contributor

Semantic ambiguity could be addressed in docstrings.

@fosskers
Copy link
Contributor

+1 interpretAs. I think it's the most semantically clear.

@pomadchin
Copy link
Member

pomadchin commented Oct 27, 2016

I like the word coercion but it usually means an implicit type conversion the word cast used for explicit (correct me if I'm wrong). So probably interpretAs or even cast (but cast looks similar to convert). But I see no problems with withCellType name as well, as for me it means more a type interpretation rather than conversion :))

  • +1 interpretAs

@echeipesh echeipesh changed the title Add interpret and asRawTile methods to Tile interface Add interpretAs and withNoData methods to Tile Oct 27, 2016
@echeipesh
Copy link
Contributor Author

Thank you everybody for chiming in on the naming!
For eons let it written, let it be said: Tile.interpretAs

@lossyrob lossyrob merged commit 64ca61d into locationtech:master Oct 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants