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.damflood: Update manual and migrate Italian to English #683

Open
wants to merge 26 commits into
base: grass8
Choose a base branch
from

Conversation

echoix
Copy link
Member

@echoix echoix commented Jan 22, 2022

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.

/* 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 */
Copy link
Member Author

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?

Copy link
Member Author

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


// DESCRIPTION OF METHOD
// First cycle: Calculation of new water heights at time t + 1:
// - Downstream of the dam: Apply continuity equation to shallow water (?)
Copy link
Member Author

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 Show resolved Hide resolved
// - 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
Copy link
Member Author

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?

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...

src/raster/r.damflood/SWE.c Outdated Show resolved Hide resolved
src/raster/r.damflood/main.c Outdated Show resolved Hide resolved
src/raster/r.damflood/r.damflood.html Outdated Show resolved Hide resolved
Comment on lines +37 to +38
In case of numerical stability problems, the user is warned, and the
simulation is stopped.<br>
Copy link
Member Author

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.

Comment on lines +129 to +133
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.
Copy link
Member Author

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"

src/raster/r.damflood/r.damflood.html Show resolved Hide resolved
@veroandreo
Copy link
Contributor

@rmarzocchi84 would you mind assisting with @echoix translation questions above?? Much appreciated! :)

@rmarzocchi84
Copy link
Contributor

@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 !!

@echoix
Copy link
Member Author

echoix commented Jan 26, 2022

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 echoix marked this pull request as draft January 26, 2022 13:51
@neteler
Copy link
Member

neteler commented Apr 24, 2022

@echoix Do you see a chance to complete this PR? Thanks!

@echoix
Copy link
Member Author

echoix commented Apr 24, 2022

Thanks for the heads up!
In fact the suggestions added were all applied, I was waiting for a return for the rest of the review from @rmarzocchi84, but it didn't came through yet.

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.

@echoix echoix force-pushed the r.damflood-patch-1 branch from 5102c66 to d13187d Compare December 30, 2023 18:49
@echoix echoix marked this pull request as ready for review December 30, 2023 18:59
@echoix
Copy link
Member Author

echoix commented Dec 30, 2023

@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.

Copy link
Contributor

@veroandreo veroandreo left a 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.

src/raster/r.damflood/SWE.c Outdated Show resolved Hide resolved
src/raster/r.damflood/SWE.c Outdated Show resolved Hide resolved
src/raster/r.damflood/main.c Outdated Show resolved Hide resolved
src/raster/r.damflood/main.c Outdated Show resolved Hide resolved
src/raster/r.damflood/main.c Outdated Show resolved Hide resolved
@echoix
Copy link
Member Author

echoix commented Dec 31, 2023

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.

@echoix
Copy link
Member Author

echoix commented Dec 31, 2023

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)

@@ -1,27 +1,26 @@
float velocita_breccia(int i, double h);
Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

@echoix echoix left a comment

Choose a reason for hiding this comment

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

Clang format

src/raster/r.damflood/main.c Outdated Show resolved Hide resolved
@lucadelu
Copy link
Contributor

@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 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 or @massimiliano-cannata can help here

@massimiliano-cannata
Copy link

massimiliano-cannata commented Dec 31, 2023 via email

@@ -1,27 +1,26 @@
float velocita_breccia(int i, double h);
Copy link
Contributor

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

Comment on lines -338 to +385
// abbassamento lago (siccome c'e due volte fare poi una function)
// Lake depth reduction (as there is twice do then a function)
Copy link
Contributor

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

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.

7 participants