Skip to content
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

Open
wants to merge 11 commits into
base: next-version
Choose a base branch
from
Open

add DHMV API #12

wants to merge 11 commits into from

Conversation

falkmielke
Copy link

Adding functionality to query DHMV (Digital Elevation Model Flanders), with a vignette to explain query construction in detail.

- dhmv api support
- adjusted roxygen
- more assertions
- version constraint
- case-insensitive wcs
@falkmielke
Copy link
Author

(checks seem to fail due to an issue in the other vignette: #13 )

@hansvancalster hansvancalster changed the base branch from main to next-version October 16, 2024 15:22
@hansvancalster
Copy link
Collaborator

I have changed the base branch to next-version

bbox,
layername,
resolution,
wcs_crs = "EPSG:4258",
wcs_crs = c("EPSG:4258", "EPSG:31370"),
Copy link
Collaborator

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.

Copy link
Author

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?

Comment on lines 149 to 153
- 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:&#x2026;"*
- *"not recognized as being in a supported file format. (GDAL error 4)"*
Copy link
Collaborator

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

Comment on lines +168 to +169
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).

Copy link
Collaborator

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... 🤔

Copy link
Author

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.

Copy link
Collaborator

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.

@hansvancalster
Copy link
Collaborator

(checks seem to fail due to an issue in the other vignette: #13 )

That should now be solved in the next-version branch. You can do git pull origin next-version with your branch checked out to align your branch with that branch.

@falkmielke
Copy link
Author

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`
@falkmielke
Copy link
Author

Changes are incorporated, vignettes build fine, rebase was a mess (sorry!).
TODOs:

  • tame the checklist package
  • double check the crs issue you mentioned above.

Again, thank you for the comments and suggestions @hansvancalster !

@hansvancalster
Copy link
Collaborator

Can you update your branch with next-version? I have merged #15 (it should also fix some check_package() issues, remaining onces - if any - are due to changes in your branch.)

Also some TO DOs:

  • add yourself as contributor
  • add a NEWS item that mentions the addition of DHMV as an additional WCS service and also mention the new vignette
  • the new vignette probably also needs some reworking in light of the work in Patch mht #15 which implies that v2.0.1 can now be used

@hansvancalster
Copy link
Collaborator

Can you update your branch with next-version? I have merged #15 (it should also fix some check_package() issues, remaining onces - if any - are due to changes in your branch.)

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).

@falkmielke
Copy link
Author

  • Can you update your branch with next-version?
  • add a NEWS item that mentions the addition of DHMV as an additional WCS service and also mention the new vignette
  • add yourself as contributor
  • the new vignette probably also needs some reworking in light of the work in Patch mht #15 which implies that v2.0.1 can now be used

I think I cannot add myself to the project, would you be so kind, @hansvancalster ?

The dhmv_api branch was already referring to mht; I brought in the latest additions (this time via merge main). Vignettes compile well with that branch installed. Checks are running, I am exploring checklist, but you might be quicker.

Thank you for reviewing!

@falkmielke
Copy link
Author

Status: I have run checklist locally and worked through the first errors (except for some tolerable, remaining spell checks, such as "Onkelinx"). Codemetar has the highest opinion of this package.
Yet the checks cannot run through: ows4R is not found. And I hesitate to add it to checklist.yml/required: it is only needed for the vignette and not a hard requirement.
Thank you for help!

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.

2 participants