Skip to content

Conversation

@PaulaSp3
Copy link
Contributor

@PaulaSp3 PaulaSp3 commented Nov 29, 2024

  • fix some small bugs (for log file)
  • fix the bug in the flux distribution (including a Flag for switching on the 'old/wrong' version) - I did not write it in the documentation yet, I'm not sure if it is necessary?
  • adapt the documentation: possible output files
  • add two new output rasters: routing flux summed up over all paths (routFluxSum) & deposited flux summed up over all paths (depFluxSum)
  • Added docstrings to function. (I almost didn't change existing docstrings/ documentation)

@PaulaSp3 PaulaSp3 requested a review from ahuber-bfw November 29, 2024 13:07
@pep8speaks
Copy link

pep8speaks commented Nov 29, 2024

Hello @PaulaSp3! Thanks for updating this PR.

Line 616:13: E265 block comment should start with '# '
Line 615:13: E265 block comment should start with '# '
Line 564:121: E501 line too long (121 > 120 characters)
Line 343:121: E501 line too long (121 > 120 characters)

Line 121:5: E722 do not use bare 'except'
Line 112:5: E722 do not use bare 'except'
Line 87:5: E722 do not use bare 'except'

Comment last updated at 2025-01-16 09:53:57 UTC

@PaulaSp3 PaulaSp3 force-pushed the PS_FP_fluxSum branch 2 times, most recently from 13c09b5 to f1e10e8 Compare December 4, 2024 11:09
Copy link
Contributor

@ahuber-bfw ahuber-bfw left a comment

Choose a reason for hiding this comment

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

  • Thx for the docstrings 👍
  • Results with new/old flux distribution version look similar enough 👍
  • routFluxSum is a cool new output file 👍 :)
  • I'm not 100% sure how to interpret the depFluxSum output yet
  • looks fine apart from minor changes

@PaulaSp3 PaulaSp3 force-pushed the PS_FP_fluxSum branch 2 times, most recently from 889ce5f to 10cbb9d Compare January 13, 2025 09:25
Copy link
Contributor

@ahuber-bfw ahuber-bfw left a comment

Choose a reason for hiding this comment

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

Looks ok from my side now!

  • apart from the flux-conservation the PR does not introduce any major changes to the code, but adds some useful improvements (documentation, docStrings, etc.)
  • @PaulaSp3: maybe you can run some tests if the checkInputParameterValues function behaves as expected - I only checked/tested this function with the global parameters (which works) - but not with the variable input rasters!

@fso42 fso42 self-assigned this Jan 14, 2025
@fso42 fso42 added the enhancement New feature or request label Jan 14, 2025
@fso42 fso42 added this to the Version 1.10 milestone Jan 14, 2025
lwdtirol and others added 2 commits January 16, 2025 10:53
  - check input parameters for sensible values
  - changes to checkInputParameterValues() and one-line in flowCore.py
@fso42
Copy link
Contributor

fso42 commented Jan 16, 2025

Standardtests ok, apart from Kot, Hof

@fso42 fso42 changed the title [com4]: Add an output raster: sum of flux Add an output raster: sum of flux [com4] Jan 16, 2025
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 1606e1a and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 2.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 69.2% (-0.2% change).

View more on Code Climate.

@fso42 fso42 merged commit 27d5bdb into master Jan 16, 2025
2 of 4 checks passed
@fso42 fso42 deleted the PS_FP_fluxSum branch January 16, 2025 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants