-
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
add DHMV API #12
base: next-version
Are you sure you want to change the base?
add DHMV API #12
Conversation
- dhmv api support - adjusted roxygen - more assertions - version constraint - case-insensitive wcs
(checks seem to fail due to an issue in the other vignette: #13 ) |
I have changed the base branch to |
bbox, | ||
layername, | ||
resolution, | ||
wcs_crs = "EPSG:4258", | ||
wcs_crs = c("EPSG:4258", "EPSG:31370"), |
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 think it is crucial that the correct EPSG code is passed here. This is the CRS that is used in the WCS to store the raster data. I noticed that using the default EPSG:4258 with DHMV resulted in raster image that was shifted compared to the corresponding wcs = "dtm" alternative.
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.
Interesting. Yet shift alone does not indicate which one is wrong.
And actually, I don't understand why we should make the "input" crs explicit. Shouldn't the web API know by itself what crs is used, return it, and only upon user request apply transformation to an output_crs
?
vignettes/spatial_dhmv_query.Rmd
Outdated
- I receive a `tif` file (because *I called it* "tif"), supposedly some kind of GeoTIFF image (because I *asked* for `image/tiff`). | ||
- I also receive a couple of error messages and warnings: | ||
- *"Start tag expected, '<' not found"* | ||
- *"cannot open this file as SpatRaster:…"* | ||
- *"not recognized as being in a supported file format. (GDAL error 4)"* |
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.
https://docs.fileformat.com/web/mht/ this is the format that is downloaded
Opening the file with a text editor such as [vim](https://www.vim.org), we see an XML part, some generic binary stuff, some hints that this [should be a GML](https://en.wikipedia.org/wiki/Geography_Markup_Language). | ||
|
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.
After the xml part, the geoTiff part starts. It is very tricky however to know exactly where this starts and ends. This is what I have attempted to do with unpack_mht() function. However, that function failed on the download from the URL that you discuss here. The reason is likely an extra white line between the xml and geotiff part... 🤔
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.
Thank you for figuring out!
I will test this in a few minutes.
Note, however, that I would be hesitant to introduce unpack_mht()
-like parsing. More of a philosophical reasoning: if sf
cannot handle the data format, then why should this convenience package of ours support it? The parser is bound to break (again), because upstream format is not standardized.
Instead, I would tend to report to DV and ask to get their file formats up to standards. (Or, more generally, document how to write correct queries.)
That written, I'm glad your parser exists, and will probably use it.
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 agree that the parser should in principle not be needed. I remember having reported this to DV developers, but the forum is no longer online unfortunately.
That should now be solved in the |
I will update this branch with the other changes you implemented, and then test DHMV v2.0.1 and mht. Thanks a lot for your work, @hansvancalster ! |
- dhmv api support - adjusted roxygen - more assertions - version constraint - case-insensitive wcs
+ rebased to follow the latest dev by @hansvancalster on parallel branches + incorporated review suggestions + modified `unpack_mht()`: the function will now return the tif path + additional vignette paragraph: using DHMV `v2.0.1`
Changes are incorporated, vignettes build fine, rebase was a mess (sorry!).
Again, thank you for the comments and suggestions @hansvancalster ! |
Can you update your branch with Also some TO DOs:
|
I note that this will result in merge conflicts; please make sure that you do not overrule the changes from the next-version branch (unless there is good reason). Otherwise, you will likely create new check_package() issues. For instance, in the Rmd you have some Dutch text between quotation marks - which will result in spelling issues. I have changed quotation marks to backticks, to avoid English spell checking on these Dutch text fragments (see https://inbo.github.io/checklist/articles/spelling.html). |
I think I cannot add myself to the project, would you be so kind, @hansvancalster ? The Thank you for reviewing! |
Status: I have run |
Adding functionality to query DHMV (Digital Elevation Model Flanders), with a vignette to explain query construction in detail.