-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement geo upload #47
base: master
Are you sure you want to change the base?
Conversation
Did you find the overlay? Where do I have to look? 😃
I saw that but I expected this to produce something else than the black/white image. There is no 0-255 range in the tiled image, just 0 and 255. |
Ahh, yeah it is a fairly small example-TIFF! The coordinates in EPSG:3857 are: -325165.2, 7779955.1 (Loch Davan close to Dinnet, Scotland)
I see, so I should check what goes wrong in the normalization. |
The formula used for image normalization should be working. It seems that the derived min- and max-values from libvips are not providing the correct min/max colour-band values as I expected them to be (i.e. they are different to what I see in the QGIS colour settings). Moreover, I tested with two different geoTIFF's and surprisingly the min/max of the two geoTIFF's were identical (min: |
You should try it with the Vips CLI too. If this does not show the expected values you can ask in the Vips repo. The maintainer is extremely responsive and helpful if you provide a good description of the problem and an example file. |
I tried with the CLI and get the correct |
(please use @mzur, otherwise I don't get notified of activity here) If this can be fixed by creating the geoTIFF in any other way then we could add an explanation to the manual and just expect geoTIFFs with the correct format. Then there is no need for special handling of edge cases (for now). |
I tested the WMS now and it works fine. I noticed an error on the geo map of a volume (not the map in the filter modal). When I open the label trees tab there, it says |
…nated single arithmetic operations.
I've implemented the color normalization such that NoData values of -99999 will get excluded in order to calculate the true min (see conversation on libvips discussion). I therefore would add an excerpt to the manual stating that color-band normalization should either be done beforehand or the NoData values of the geoTIFF should be set to -99999 in order to be handled correctly by BIIGLE. |
Sounds good 👍 |
That's fixed now.
Regarding the latter, I tried to find where the warning is coming from by debugging the code, but I couldn't figure out what causes it. It is still unclear to me...
I looked into the whole upload process of the geoTIFF files, comprised of: and commenting lines step by step to see where the warning stems from, but it's still not obvious to me. Do you have any idea or instinct what could cause such warning? |
The error came from the The issue is described here: LinusBorg/portal-vue#201 |
Thanks for the help and clarification on this issue! 👍 |
@@ -46,7 +42,7 @@ export default { | |||
data.append('volumeId', this.volumeId); | |||
geoApi.saveWebMap({id: this.volumeId}, data) | |||
.then(this.handleSuccess, this.handleError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I currently get a 500 server error if the WMS URL is invalid.
- The 500 error should not occur. instead it should return a 422 validation error.
- Even if a 500 occurs, an error message should be shown in the UI.
@@ -104,7 +104,7 @@ protected function getVipsImage($path) | |||
*/ | |||
protected function imageNormalization($vipsImage, $min, $max) | |||
{ | |||
// band intensity normalization x' = (x - $min * (255 / ($max - $min)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the position is also not 100% correct but I guess this is the error we talked about and which can't be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This produces undesired console output from processing actual files. Maybe this can be mocked?
$table->foreign('volume_id') | ||
->references('id') | ||
->on('volumes') | ||
->onDelete('cascade'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need an event handler/observer that deletes the geo overlay files when the whole volume or project is deleted.
@@ -40,10 +40,14 @@ public function boot(Modules $modules, Router $router) | |||
'viewMixins' => [ | |||
'imagesIndex', | |||
'manualTutorial', | |||
'manualVolumes', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not manualVolumes
mixin location on the manual index page. It can be added in biigle/core#725, though.
There is also the volume-map
manual article. The description of the geo filter there is no longer correct.
$router->get('geo-overlays/{id}/file', [ | ||
'uses' => 'GeoOverlayController@showFile', | ||
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used?
$faker = \Faker\Factory::create(); | ||
$model = new GeoOverlay; | ||
$model->name = $faker->company(); | ||
$model->volume_id = VolumeTest::create()->id; | ||
$model->type = 'geotiff'; | ||
$model->browsing_layer = false; | ||
$model->context_layer = false; | ||
$model->layer_index = null; | ||
$model->attrs = [ | ||
'top_left_lng' => $faker->randomFloat(), | ||
'top_left_lat' => $faker->randomFloat(), | ||
'bottom_right_lng' => $faker->randomFloat(), | ||
'bottom_right_lat' => $faker->randomFloat(), | ||
'width' => $faker->randomNumber(), | ||
'height' => $faker->randomNumber() | ||
]; | ||
$model->save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better define a factory for the model.
$model = self::createGeotiffOverlay(); | ||
$model->volume->delete(); | ||
$this->assertNull($model->fresh()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also test if the overlay files are removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are quite slow because they process actual files. Maybe this can also be mocked (e.g. implement a method "read exif" that returns static data in the test without opening an actual file)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure that this test does not actually send HTTP requests!
references #44
Implement the structure for the upload mechanism in the volume-edit view. Allow the upload of both - browsing layers and context layers - i.e. upload of geoTIFF and (iFDO) metadata, linkage of WMS.
associated PR's: