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

allow larger maps in r.terraflow #265

Merged
merged 2 commits into from
May 3, 2020
Merged

Conversation

ninsbl
Copy link
Member

@ninsbl ninsbl commented Dec 19, 2019

Replaces #31
patch with a fix provided by @metzm

@ninsbl
Copy link
Member Author

ninsbl commented Dec 19, 2019

Unfortunately, also memory seems to be Limited by some sort of type definition.

If I allocate a lot of the sytem memory (100GB) on a large map, so out-of-memory computation is still necessary, I get the following error message:

terminate called after throwing an instance of
'std::bad_array_new_length'
  what():  std::bad_array_new_length

@wenzeslaus
Copy link
Member

Slightly off topic:

patch with a fix provided by @metzm

For co-authors, I suggest using Co-authored-by at the end of the commit message:


Co-authored-by: name <name@example.com>

I think empty line (i.e., two new lines) is necessary and sufficient (GitHub documentation is little confusing there, but the generally that's what's accepted).

@wenzeslaus
Copy link
Member

According to cppreference.com, std::bad_array_new_length happens when array length is negative (the other two options don't look likely in our case). Since number of rows and columns seems to be (always?) defined as dimension_type which is a typedef for short (types.h:29), I would guess that it overflows, becomes negative, and if that eventually goes to new in one way or the other, the exception is thrown. Although, there is if ((nr > dimension_type_max) || (nc > dimension_type_max)) test in main.cpp:486 which is trying to fail fast there.

@metzm
Copy link
Contributor

metzm commented Dec 20, 2019

dimension_type has been changed in this PR from short to int, but this is only part of the solution. In order to support large maps, all instances where rows * cols is computed must cast to long, or better some portable 64 integer type.

I guess I overlooked an instance where rows * cols is computed, I will check again.

@neteler
Copy link
Member

neteler commented Dec 23, 2019

Slightly off topic:
For co-authors, I suggest using Co-authored-by at the end of the commit message:


Co-authored-by: name <name@example.com>

(added to https://trac.osgeo.org/grass/wiki/HowToGit#Citingco-authorsinagitcommitmessage )

@ninsbl
Copy link
Member Author

ninsbl commented Jan 16, 2020

Desired functionality is also available in r.hydrodem, which is able to handle large rasters.

The question is how to proceed with this PR.
We can close it without merging, since other options exist. Or - if we consider the changes still relevant - we merge the PR as an improvement of the current code, though not perfect (memory is limited to 32GB).
If we decide not to merge, we should probably consider removing r.terraflow from core and adding/prommoting r.hydrodem instead?

In either case we should close the related trac ticket...

@metzm
Copy link
Contributor

metzm commented Jan 16, 2020

Memory in r.terraflow is limited to 32 GB? Interesting. This must come from the GRASS iostream library and would also affect r.viewshed. We do not have an alternative for r.viewshed, therefore the memory limit should be fixed.
I opt to merge this PR and create a new issue/ticket for the memory limit.

@neteler
Copy link
Member

neteler commented Apr 26, 2020

I opt to merge this PR and create a new issue/ticket for the memory limit.

@ninsbl will you merge this PR?

@ninsbl ninsbl merged commit 75ac17f into OSGeo:master May 3, 2020
@ninsbl ninsbl deleted the r_terraflow_fix_mm branch May 3, 2020 20:16
@neteler neteler added this to the 7.10 milestone May 3, 2020
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.

None yet

4 participants