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

r.sim.water: add info to manual, add info about progress #4662

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Nov 6, 2024

  • r.sim.water is a module with a number of important but not well documented parameters. This PR adds more information about some parameters to the manual.
  • It takes quite some time to run a simulation, another message is added to the progress of the simulation.

This PR tries to add info requested in #4475 to the manual

@metzm metzm added manual Documentation related issues raster Related to raster data processing C Related code is in C labels Nov 6, 2024
@metzm metzm added this to the 8.5.0 milestone Nov 6, 2024
@metzm metzm requested a review from petrasovaa November 6, 2024 21:39
@github-actions github-actions bot added HTML Related code is in HTML module docs labels Nov 6, 2024
@metzm metzm requested a review from petrasovaa November 7, 2024 21:25
@hmitaso
Copy link
Contributor

hmitaso commented Nov 9, 2024

This sentence may be confusing "The model is for shallow water flow, where "shallow" is defined by hmax"- Perhaps the paragraph could start with "the model tries to keep water "shallow" with maximum shallow water depth defined by hmax value (default is 0.3m). If hmax is reached then .... here goes the text that explains the increase in diffusion rate and related parameters .... The rest of the text that explains when these parameters may need to be adjusted sounds good to me.

@metzm
Copy link
Contributor Author

metzm commented Nov 13, 2024

This new description comes from #4475, maybe @petrasovaa can clarify? I am not sure if the model tries to keep water "shallow". After hmax is reached, water flow can more easily overcome obstacles like sinks. "shallow" might here refer not to water depth, but to noise or sinks in the DEM used as input.

@metzm
Copy link
Contributor Author

metzm commented Nov 14, 2024

@hmitaso New try to explain diffusion in order to keep water shallow and to overcome obstacles in ad725c7.

@neteler neteler changed the title r.sim.water: add info to manual, add info about progress [Feat] r.sim.water: add info to manual, add info about progress Jan 6, 2025
metzm and others added 2 commits January 9, 2025 21:19
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@neteler neteler changed the title [Feat] r.sim.water: add info to manual, add info about progress r.sim.water: add info to manual, add info about progress Jan 10, 2025
neteler
neteler previously approved these changes Jan 10, 2025
Copy link
Member

@neteler neteler left a comment

Choose a reason for hiding this comment

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

Locally tested, looks good to me.

@neteler
Copy link
Member

neteler commented Jan 10, 2025

May this be merged, @petrasovaa ?

@@ -286,7 +286,9 @@ int main(int argc, char *argv[])
parm.threads->answer = NUM_THREADS;
parm.threads->required = NO;
parm.threads->description =
_("Number of threads which will be used for parallel compute");
_("Number of threads which will be used for parallel computation. "
"Increasing the number of threads does not really speed up "
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this from here, maybe clarify in documentation or add benchmark there. Based on my experience (with r.sim.water, I don't know about r.sim.sediment) a few cores provide somespeed up, although I agree more than 4 cores probably not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with bbf9117

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate on this? In your experience, there is no speedup at all? Because that's what the comment suggests. For sure it doesn't scale well, but at least for 4 threads the authors report 3x speedup.

raster/r.sim/r.sim.water/main.c Outdated Show resolved Hide resolved
raster/r.sim/simlib/hydro.c Show resolved Hide resolved
raster/r.sim/r.sim.water/main.c Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C docs HTML Related code is in HTML manual Documentation related issues module raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants