Skip to content

[GL] Enlarge delta for grid meta data computation.#1369

Merged
TomFischer merged 1 commit intoufz:masterfrom
TomFischer:FixGrid
Aug 29, 2016
Merged

[GL] Enlarge delta for grid meta data computation.#1369
TomFischer merged 1 commit intoufz:masterfrom
TomFischer:FixGrid

Conversation

@TomFischer
Copy link
Copy Markdown
Member

As titled.

@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented Aug 24, 2016

Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/2661/

@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented Aug 24, 2016

Jenkins: OGS-6/Linux-PRs-dynamic failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs-dynamic/884/

@TomFischer
Copy link
Copy Markdown
Member Author

Jenkins, test this please.

@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented Aug 24, 2016

Jenkins: OGS-6/Linux-PRs-dynamic failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs-dynamic/894/

@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented Aug 24, 2016

Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/2671/

@TomFischer
Copy link
Copy Markdown
Member Author

Jenkins, test this please.

Comment thread GeoLib/Grid.h

// enlarge delta
for (auto & d : delta)
d = std::nextafter(d, std::numeric_limits<double>::max());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

delta is the space diagonal of the bbox of the grid? Why do you have to extend it such a tiny bit?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, delta is the space diagonal of the bounding box and also of the grid. The bounding box itself is extended already such a tiny bit during its construction. But when delta is computed from the bounding box the tiny extension of the bounding box could get lost because of numerical precision. So here the delta is enlarged again such that all points fits into the grid.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not really happy with this. I think the result of std::nextafter can be cancelled out by very many arithmetic operations (e.g. adding or subtracting a large number). I think multiplying with 1+eps would be more robust, but I don't know if that has any implications for our current code.
So if nobody else objects and the solution works for you: ⏩

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think the result of std::nextafter can be cancelled out by very many arithmetic operations (e.g. adding or subtracting a large number).

In general you re right. The bounding box is an half open interval. As you pointed out already out the arithmetic operations computing delta cancel out the results of std::nextafter in the bounding box. For this reason I reenlarge delta.

I think multiplying with 1+eps would be more robust ...

Sometimes all points have the same z coordinate, i.e., the delta[2] == 0. So the half open interval has to be enlarged. In this case the multiplication with 1+eps does not lead to the desired solution.

@endJunction
Copy link
Copy Markdown
Member

@TomFischer TomFischer merged commit 6513dbc into ufz:master Aug 29, 2016
@TomFischer TomFischer deleted the FixGrid branch August 29, 2016 06:16
@ogsbot
Copy link
Copy Markdown
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

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.

4 participants