Skip to content

Add interpretAs and withNoData methods to Tile - #1702

Merged
lossyrob merged 9 commits into
locationtech:masterfrom
echeipesh:feature/eac/tile-interpret
Oct 30, 2016
Merged

lossyrob merged 9 commits into
locationtech:masterfrom
echeipesh:feature/eac/tile-interpret

Conversation

@echeipesh

@echeipesh echeipesh commented Oct 25, 2016 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

Is this WIP until the RFC gets worked out?

@echeipesh

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

moradology commented Oct 27, 2016 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

Semantic ambiguity could be addressed in docstrings.

@fosskers

Copy link
Copy Markdown
Contributor

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

@pomadchin

pomadchin commented Oct 27, 2016 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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