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.horizon: get rid of global variables and other refactoring #3346

Merged
merged 17 commits into from
Mar 22, 2024

Conversation

petrasovaa
Copy link
Contributor

I refactored r.horizon to make it possible to extend it in the future (e.g. multiple coordinates, parallelization, etc.), including

  • removing most global variables and reorganizing them into structures depending on when the variables change and when they don't, e.g. member variables in new struct OriginAngle are constant for each origin point and angle computed.
  • reducing the scope of some local variables
  • renaming/reorganizing some functions
  • reducing the code duplication between the point and raster mode
  • removing unused functions
  • addressing cppcheck complaints
  • adding a test comparing values of raster mode result with point mode results for couple points

I tried not do any changes that could impact results, e.g. there is still some discrepancy in how maxlength is computed for the modes.

@petrasovaa petrasovaa added this to the 8.4.0 milestone Jan 8, 2024
@petrasovaa petrasovaa requested a review from wenzeslaus January 8, 2024 18:43
@petrasovaa petrasovaa self-assigned this Jan 8, 2024
@github-actions github-actions bot added raster Related to raster data processing Python Related code is in Python C Related code is in C module labels Jan 8, 2024
@echoix
Copy link
Member

echoix commented Feb 8, 2024

Does this PR need to be updated following the recent changes in r.horizon?

Yes, but they are not all merged yet.

@echoix
Copy link
Member

echoix commented Feb 14, 2024

@petrasovaa I didn't see your reply last week as you directly edited my comment instead of replying.

@github-actions github-actions bot added the tests Related to Test Suite label Feb 16, 2024
@petrasovaa petrasovaa modified the milestones: 8.4.0, 8.5.0 Feb 16, 2024
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

I applaud this initiative. Could/should be done on most other modules.

Briefly find a few minor issues, commented in line.

raster/r.horizon/main.c Outdated Show resolved Hide resolved
raster/r.horizon/main.c Outdated Show resolved Hide resolved
raster/r.horizon/main.c Outdated Show resolved Hide resolved
@petrasovaa
Copy link
Contributor Author

I applaud this initiative. Could/should be done on most other modules.

Briefly find a few minor issues, commented in line.

Thank you, I applaud you reviewing this!

nilason
nilason previously approved these changes Feb 21, 2024
Copy link
Member

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

Looks good now, no new comments to address, launched a new round of CI to make sure it still works, but other than that, it seems ready to merge.

@petrasovaa petrasovaa merged commit 8c56397 into OSGeo:main Mar 22, 2024
25 checks passed
@petrasovaa petrasovaa deleted the r.horizon-rewrite branch March 22, 2024 14:06
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 module Python Related code is in Python raster Related to raster data processing tests Related to Test Suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants