-
Notifications
You must be signed in to change notification settings - Fork 157
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.damflood: Update manual and migrate Italian to English #683
base: grass8
Are you sure you want to change the base?
Conversation
…addons into r.damflood-patch-1
src/raster/r.damflood/SWE.c
Outdated
/* controlla Q=0.0 & volume=0.0 */ | ||
/* TO BE PLACED IN ADDITIONAL FUNCTION IN fall.c */ | ||
/* called both here and in main */ | ||
/* Check (for) Q=0.0 & volume=0.0 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it check, check for, checking, etc that is meant for the last line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there is no fall.c here
src/raster/r.damflood/SWE.c
Outdated
|
||
// DESCRIPTION OF METHOD | ||
// First cycle: Calculation of new water heights at time t + 1: | ||
// - Downstream of the dam: Apply continuity equation to shallow water (?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure of this sentence, need to remove (?) when someone understands the original meaning
src/raster/r.damflood/SWE.c
Outdated
// - Upstream of the dam: | ||
// - In methods 1 and 2: | ||
// - The continuity equation is applied to the volume of the lake | ||
// Physically this leads to a less realistic but avoids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't make much sense, but it was the closest that kept the same meanings of the Italian comment. Someone has a better explication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think swe applies where speed in plane directions are of a magnitude higher than in vertical direction, and in a lake it is questionable...
In case of numerical stability problems, the user is warned, and the | ||
simulation is stopped.<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a hard time deciding if this was the correct interpretation, given that code is speaking of "Courant-Friedrich-Lewy stability condition" and seems specific to the numerical algorithm. I tried to learn more on this, but I'm still not quite sure. This sentence refers to (now) lines 889-895 of main.c, comments at (now) lines 84-94 of SWE.c, code and comments of lines 550-536 and 724-742 of SWE.c.
Optionally the user may modify the initial timestep used for the numerical | ||
solution of the SWE (<em>default value = 0.01 s</em>), nevertheless the | ||
timestep [<b>timestep</b>], | ||
and choose a specific failure type corresponding to the different computational | ||
methods for the initial velocity estimation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of what was meant about the original "nevertheless"
Co-authored-by: Nicklas Larsson <[email protected]>
@rmarzocchi84 would you mind assisting with @echoix translation questions above?? Much appreciated! :) |
I will finish the review tomorrow! Many thanks for your work @echoix really appreciated !! |
My second step was to refactor the variable names to have them based on English, to finish the dependency on Italian for future contributors. Should it be done here while you are still available? In both cases, take your time and I'll apply the suggestions this weekend. |
@echoix Do you see a chance to complete this PR? Thanks! |
Thanks for the heads up! I still have the question of "do we try to refactor Italian variable names" in the scope of this PR to help keep the addon maintainable while the origin author is still accessible? The draft status was to not make it ready to merge, since we were waiting for a response. |
5102c66
to
d13187d
Compare
@lucadelu I was wondering if you would mind helping out with the italian here. Its one of my first PRs two years ago, and never got to finish it up yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor fixes in translation. I still have problems understanding L385 and the word stramazzo. I did not check the html file as I am not familiar with the topic.
Is stramazzo from https://it.m.wikipedia.org/wiki/Stramazzo the same as spillway from https://en.m.wikipedia.org/wiki/Spillway? Html had way less changes, it was already in English. |
Sorry, it's quite hard to review now, since the changes started before the code was formatted, and it was really hard to solve the conflicts (I merged in several passes since trying to merge from the current grass8 branch was too off) |
Co-authored-by: Veronica Andreo <[email protected]>
@@ -1,27 +1,26 @@ | |||
float velocita_breccia(int i, double h); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is velocita_breccia the velocity of breach, break, or another concept?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deepl also suggest gap, hole
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang format
@echoix I checked the file and looks good to me, I have no idea about translation of |
Hi there,
Stramazzo in English is
spillway, or weir
Maxi
Il dom 31 dic 2023, 09:17 Luca Delucchi ***@***.***> ha
scritto:
… @lucadelu <https://github.com/lucadelu> I was wondering if you would mind
helping out with the italian here. Its one of my first PRs two years ago,
and never got to finish it up yet.
@echoix <https://github.com/echoix> I checked the file and looks good to
me, I have no idea about translation of stramazzo because I don't really
know what is it in Italian too. Maybe @rmarzocchi84
<https://github.com/rmarzocchi84> or @massimiliano-cannata
<https://github.com/massimiliano-cannata> can help here
—
Reply to this email directly, view it on GitHub
<#683 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADC2FTAHTUZIBZJFEOKIHXTYMENR7AVCNFSM5MSGRRPKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBXGI4DGOBQGM3Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@@ -1,27 +1,26 @@ | |||
float velocita_breccia(int i, double h); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deepl also suggest gap, hole
// abbassamento lago (siccome c'e due volte fare poi una function) | ||
// Lake depth reduction (as there is twice do then a function) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment between parenthesis makes no sense to me
The changes in this addon were made in a separate PR since they were more important than others.
Before merging, I'd like some attentive feedback from Italian-speaking contributors to validate that the comments have the same meaning. I tried my best to find something that works in the code context.