-
Notifications
You must be signed in to change notification settings - Fork 196
Fix TerrainRGB algorithm and param user-controlled nodata-height #1116
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
Conversation
Added two params `use_nodata_height and nodata_height` (or could have used a field which cold be either undefined or that height value?
defaults to 1, applied to gradients directly
Co-authored-by: Vincent Sarago <[email protected]>
Co-authored-by: Vincent Sarago <[email protected]>
Co-authored-by: Vincent Sarago <[email protected]>
Co-authored-by: Vincent Sarago <[email protected]>
we will need to update the tests in titiler/src/titiler/core/tests/test_algorithms.py Lines 69 to 107 in 57b44d6
|
I'm usually testing with |
Adding algorithm params dynamically based on the /algorithms endpoint, stored in scope, and params updated on change of selected algorithm
Uses number inputs if param is integer or number, otherwise text input (eg nodata-height which can be either null or number) Updates tilejson url when algorithm or its params are changed
I think we should decouple the e.g algorithm might need more than one band, so I don't think they should be in the |
FYI I've started a PR in rio-viz to support raster-dem viz developmentseed/rio-viz#64 ;-) |
Wow that was fast, 3d maplibre support looking nice! I can revert the changes specific to the viewer in that titiler PR, should I port the algorithms selection to another PR in Rio-viz instead? Still a wip, it can indeed be useful to other bands as well, will need to strengthen that wip. |
@jo-chemla I've personally don't have a lot of time to work on this (I've already spend too much time 🙈) so I would say you can but don't expect too much 😅 |
Alright, thanks again for getting back! I've reverted commits dedicated to Trying to gather what's missing before merging:
I think I can PR the /cog/viewer in a new PR almost as-is, and
|
Yeah they are different templates (because at the beginning they were quite different but with time they evolved to be a bit too close now 😓 let's not worry about rio-viz for now, you can focus on titiler cog viewer 🙏 |
Just had a quick look,
The missing tests are:
The thing is I'm not sure which test is best among the 2 solutions below. Either edit # SOLUTION 1
def test_terrain_algo():
#...
#def main(algorithm=Depends(default_algorithms.dependency)):
# client = TestClient(app)
# ...
# SOLUTION 1
# test nodata_height
# MAPBOX Terrain RGB
response = client.get("/", params={"algorithm": "terrainrgb", "algorithm_params":json.dumps({"nodata_height":10.0})})
assert response.status_code == 200
with MemoryFile(response.content) as mem:
with mem.open() as dst:
data = dst.read().astype(numpy.float64)
elevation = -10000 + (((data[0] * 256 * 256) + (data[1] * 256) + data[2]) * 0.1)
numpy.testing.assert_array_equal(elevation, arr[0])
# TILEZEN Terrarium
response = client.get("/", params={"algorithm": "terrarium", "algorithm_params":json.dumps({"nodata_height":10.0})})
assert response.status_code == 200
with MemoryFile(response.content) as mem:
with mem.open() as dst:
data = dst.read().astype(numpy.float64)
elevation = (data[0] * 256 + data[1] + data[2] / 256) - 32768
numpy.testing.assert_array_equal(elevation, arr[0])
# SOLUTION 2
def test_terrarium():
# ...
# nodata_height
nodata_height = 10.0
algo = default_algorithms.get("terrarium")(nodata_height=nodata_height)
arr = numpy.ma.MaskedArray(
numpy.random.randint(0, 5000, (1, 256, 256), dtype="uint16"),
mask=numpy.zeros((1, 256, 256), dtype="bool"),
)
arr.mask[0, 0:100, 0:100] = True
img = ImageData(arr)
out = algo(img)
masked = out.array[:, arr.mask[0,:,:]]
masked_height = (masked[0] * 256 + masked[1] + masked[2] / 256) - 32768
print(masked_height.shape)
numpy.testing.assert_array_equal(masked_height, nodata_height * numpy.ones((100 * 100), dtype="bool")) |
Yeah I think both solution are fine. I'm ok with what we have right now 👍 we don't need to worry to much about the coverage % |
Thanks, from my point of view we should be ready to go then! Also, the last item from the original discussion was to generalize algorithms to ingest not only 1b DEMs, but also allow transformation of the input data from terrarium/terrainrgb into altitude/height data (or algorithms chaining) so it could be used on-the-fly by algorithms (hillshade etc). I'm not sure how this could/should work and it can probably be the subject of another PR if deemed useful. Thanks for all the help as always! |
@jo-chemla could you run python -m pip install pre-commit
# in titiler root
pre-commit install
pre-commit run --all-files
Do you mean go from RGBA (terranium) to Float? (I think you could use band expression for this)
we could allow algorithm to be a list but then it might be a bit messy when wanting to pass options to each algorithm. FYI We've recently worked on OpenEO https://github.com/sentinel-hub/titiler-openeo and the custom processing language is quite interesting. We might bring this to titiler (if any customer need it 🙈 ) |
Thanks again. Just ran the pre-commit and committed, so should be good to go on this one! Nice to see the custom Processing language for OpenEO, first time I hear about it 👀. It does look a lot like Node editors for 3D-artist related workflows, eg in case any inspiration could be useful:
And for algorithms chaining, 🤯 indeed the band expression is the way to prepare the data before algorithm execution (eg the existing aws open-data terrain tiles which are stored as terrarium with url Makes me want to add expression support before merging #1119 |
Again I don't want to add too much stuff into the If you really want to play/visualize the data I often use the |
Alright, just tried adding the |
use_nodata_height and nodata_height
(or could have used a field which could be either undefined or that height value?).Can be tested via
TODO: Also should take a moment to add suggested updates from discussion #1110
Just noticed also the /cog/viewer endpoint to help users play with url-params, bands expressions etc. Extend single-band user-parametrization to also allow for algorithms and a few other url-params like rescale etc?other PR Add algorithms selection to /cog/viewer and custom expression support #1119