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

update getDEM to use the new dem-stitcher API #342

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

jlmaurer
Copy link
Collaborator

@jlmaurer jlmaurer commented Jun 9, 2022

Description

The dem-stitcher API changed at several points; this is catching us up.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have added an explanation of what your changes do and why you'd like us to include them.
  • I have written new tests for your core changes, as applicable.
  • I have successfully ran tests with your changes locally.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@jlmaurer
Copy link
Collaborator Author

jlmaurer commented Jun 9, 2022

Fixes #335
Replaces #339

@jlmaurer jlmaurer added the bug Something isn't working label Jun 9, 2022
@jlmaurer jlmaurer requested review from bbuzz31, sssangha and cmarshak June 9, 2022 20:05
@cmarshak
Copy link
Collaborator

cmarshak commented Jun 10, 2022

I am going to merge a major change hopefully in the coming weeks and it changes the API to how I originally had it (and ensures that we preserve the original DEM tiles as much as possible).

Given this needs to be global and you are using glo_30 there are known issues over Armenia and Azerbaijan (see this issue ticket). I have to have some logic in the DockerizedTopsApp and will add a notebook. Will be easy to include in your getDEM. We will take glo-90 tiles and upsample them to fill in the holes over these areas.

One thing that is worth noting at this point is whether you want the DEM to have geo-referencing tied to the center of the pixel ('Point' tag) or the upper left corner ('Area' tag). See the readme of dem-stitcher for some discussion about this and references.

@cmarshak
Copy link
Collaborator

cmarshak commented Jun 10, 2022

To ensure that it works with the API you are using - please fix the version - and we can go back and update it later.

@jlmaurer
Copy link
Collaborator Author

@cmarshak yes corner- versus center-referencing has been a big issue for us. Everything we do is corner-referenced (so Area tag). @dbekaert can you confirm on this?

@jlmaurer
Copy link
Collaborator Author

@cmarshak I'll get the environment files updated tomorrow to fix the version of dem-stitcher.

@cmarshak
Copy link
Collaborator

FYI - the AREA tag will likely not work correctly with the API you are using (e.g. this issue), but again, we will fix this once the new version is released.

Copy link
Collaborator

@cmarshak cmarshak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.

@jlmaurer jlmaurer merged commit 522473e into dbekaert:dev Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants