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

Feature/mct optim fix #173

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Conversation

corentincarton
Copy link
Collaborator

No description provided.

@doc78
Copy link
Collaborator

doc78 commented Oct 18, 2024

I checked the dis.tss results of the branch master and the current branch using cold.xml. Results after the commit "split and optimization of the new solve1pixel function " are bit identical. Results after "minor optimisations" are slightly different but I think they are still OK:

29.2018 -> 29.2017
25.7968 -> 25.7967
58.7482 -> 58.7476
51.9246 -> 51.9239
...

@ecCinziaMazzetti
Copy link
Contributor

Is 'minor optimisations' strictly necessary? I would very much prefer having bit identical results.

@corentincarton
Copy link
Collaborator Author

I think we can maybe do something in between that allows bit reproducibility. I think moving around some parts of the code (counts, upstream inflow, etc.) helped but maybe not the removal of closureError and the change in the while loop, which is what impacts may the bit reproducibility.

num_upstream_pixels, a_dx_div_dt[pix], b_a_dx_div_dt[pix], beta, inv_beta, b_minus_1):
solve1PixelAvg(pix, discharge_avg, discharge_before, discharge[pix], lateral_inflow[pix], upstream_lookup,\
num_upstream_pixels, inv_time_delta, beta, a_dx[pix])

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please @ecCinziaMazzetti and @StefaniaGrimaldi just check the changes here, as I think we were using the whole arrays previously in the computation, while only the "pix" element of the a_dx, constant, lateral_inflow, a_dx_div_dt and b_a_dx_div_dt arrays were needed in the solve1pixel functions, but I can be wrong.

discharge[pix] = 0
return
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ecCinziaMazzetti @StefaniaGrimaldi Here we skip the computation of the discharge_avg value, but discharge[pix] is changed, as we set it to zero. Is that correct? Should we still need to compute channel_volume_start and channel_volume_end for dischage_avg as the quantity of discharge[pix] has changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@doc78, I discussed this with @ecCinziaMazzetti a few weeks ago when reviewing the changes and this looks like the correct behaviour. No need to compute channel_volume_start and channel_volume_end and discharge_avg is not computed if discharge[pix] is 0 (solve1PixelAvg not called).

@doc78
Copy link
Collaborator

doc78 commented Nov 8, 2024

We run some test for speed and result differences adding the commit "minor optimisation" and we found the following:
Speed: there is no speed difference between the two versions (with and without minor optimization).
Results differences: compared to the feature\mct_optim, the minor optimization commit introduces differences in the order of 0.000001 (absolute values), while in the version without minor optimization the differences are less then 0.0000001 (even if both version are not bit identical to the feature\mct_optim branch).
For testing, we used catchments G0629 (GloFAS3arcmin setup), 3 years of daily simulations. The simulation included only SplitRouting optional module, all other optional modules were deactivated. The comparison used the output netcdf files.

@corentincarton
Copy link
Collaborator Author

@doc78, thanks for this! We did some checks on our side and we also came to the same conclusion. I think we can move forward with this version. @ecCinziaMazzetti and Nikos are currently running some performance and calibration tests, we'll share with you the results of the investigations.

@ecCinziaMazzetti ecCinziaMazzetti merged commit 96e0d96 into feature/mct_optim Nov 15, 2024
1 check failed
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.

3 participants