Hillshade seg map #87
Conversation
…h a slope map and a Hillshade
we can now overlay a hillshade or slope model. does not need to convert the whole dataset to EEPSG:4326
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
I liked the code that you wrote and mainly marked some minor stylistic nitpicks. The only major thing I see is the duplication of visualization configuration options instead of honouring the existing schema. If you prefer so, you could do this refactoring after the merge, but I would like to do it for sure.
| resolution=2, | ||
| azimuth=315, | ||
| angle_altitude=45, | ||
| opacity=0.6, |
There was a problem hiding this comment.
This parameter list is a sort of redundant repetition of the input parameters for visualization. Instead, it would be possible to accept **kwargs and validate it against the schema defined in schema/visualization.json. The docstring could just refer to it as "accepts all parameters that DataSet.show accepts. The non-supported visualization types (Scatter, 2.5D mesh) can be excluded by throwing an error.
| + ",az:" | ||
| + str(azimuth) | ||
| + ",ang:" | ||
| + str(angle_altitude) |
There was a problem hiding this comment.
Not sure a string is the best option here. If you have **kwargs anyway, tuple(kwargs.items()) might do the job.
| resolution = resolution | ||
|
|
||
| # calculate the hillshade or slope | ||
| if map_type == "Hillshade": |
There was a problem hiding this comment.
Again this logic is duplicated from visualization and could be reused. Maybe the dispatch from https://github.com/ssciwr/adaptivefiltering/blob/hillshade_seg_map/adaptivefiltering/dataset.py#L114 needs to be refactored to be available from visualization.py.
tests/test_segmentation.py
Outdated
| test_map = Map(segmentation=5) | ||
|
|
||
|
|
||
| def test_load_overlay(dataset_thingstaette, boundary_segmentation): |
There was a problem hiding this comment.
Is the THingstaette data set actually in the repo? I cannot find it right now (independently of our LFS problems)
There was a problem hiding this comment.
apparently it is not, but if we decide to not use the Westport dataset, we should probably include it.
There was a problem hiding this comment.
The dataset is not published under an open license, we should be looking for a replacement.
Codecov Report
@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 84.08% 84.79% +0.71%
==========================================
Files 14 14
Lines 1181 1289 +108
==========================================
+ Hits 993 1093 +100
- Misses 188 196 +8
Continue to review full report at Codecov.
|
|
Further review comments deferred to a future PR. |
#71 GeoTiff support has been added, with hillshade and slope visualization, but geoJPEG support has not yet been added.
#64 mouse wheel no longer interacts with the map.