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 a flag to allow for Geoid-height on NaN points in DEM generation #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reinieroost
Copy link

This allows to avoid NaN values at places where the DEM is NaN, by using Geoid heights instead. This is very favorable for coastal regions. It is a switch that does not affect the default.

I will also update the UI (snap-desktop), see pull-request.

@marpet
Copy link
Member

marpet commented Sep 24, 2017

Link to desktop PR: senbox-org/snap-desktop#40

@marpet
Copy link
Member

marpet commented Sep 24, 2017

Thanks for this PR. That's great. User contribution is always welcome.
I think we can't consider this for SNAP 6 release any more, but probably for a later update.

Thanks

@@ -78,6 +78,9 @@
@Parameter(description = "The elevation band name.", defaultValue = "elevation", label = "Elevation Band Name")
private String elevationBandName = "elevation";

@Parameter(description = "Fill nodata values by geod values", defaultValue = "false", label = "Fill Nodata with Geod")
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note: there's a typo in 'geoid'.

@lveci
Copy link
Contributor

lveci commented Oct 4, 2017

The snap-desktop pull request has already been merged but not this one on snap-engine. Therefore, AddElevationUI currently breaks because the fillWithGeod parameter doesn't exist in AddElevationOp.

Also there are spelling errors for fillWithGeod.

@marpet
Copy link
Member

marpet commented Oct 8, 2017

@jun--lu Why have you reverted the PR snap-desktop#40 by this senbox-org/snap-desktop#41? Why not simply applying this PR and fix the typo?

@jun--lu
Copy link
Contributor

jun--lu commented Oct 9, 2017

Hi Marco,

By accident I merged the pull request under snap/desktop on AddElevationUI to master and I did not know that there is anther pull request under snap/engine on AddElevationOp until Luis pointed out. I've tried to merge AddElevationOp to master, then I realised that you want to make this change after V6.0 release (your email on Set. 24 ). Therefore, I reversed the merge.

Jun

@marpet
Copy link
Member

marpet commented Oct 10, 2017

Jun,

if this PR is tested and does not break existing processing chains I’m fine with the merge of it.
I only said that we probably accept this PR only after 6.0 release because we had several other high priority things to fix first.
If you think this is a good one, please go ahead.

@jun--lu
Copy link
Contributor

jun--lu commented Oct 10, 2017

Hi Marco,
As I've said I made the merge really by accident and I don't think I have time to check the code and test it now. It's better to merge it after the release when we have time to deal with things like this.
Jun

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

6 participants