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

added potential evaporation to offline output, changed checks range, … #346

Conversation

har917
Copy link
Collaborator

@har917 har917 commented Jul 15, 2024

CABLE

Thank you for submitting a pull request to the CABLE Project.

Description

This PR adds the potential evaporation diagnostic (canopy%epot) to fluxes section of the cable output.
A minor correction to the diagnostic has also been applied - which could lead to impacts in the coupled model (via %wetfac_cs and JULES variable resft) - and the checks_ranges updated.

Fixes #335

Type of change

Please delete options that are not relevant.

  • Bug fix
  • additional output

Checklist

  • The new content is accessible and located in the appropriate section.
  • I have checked that links are valid and point to the intended content.
  • I have checked my code/text and corrected any misspellings

Please add a reviewer when ready for review.


📚 Documentation preview 📚: https://cable--346.org.readthedocs.build/en/346/

@har917
Copy link
Collaborator Author

har917 commented Jul 15, 2024

@ccarouge Holding off on benchcab testing for now given that MAIN is currently broken.

@AlisonBennett This changeset works for single site runs - and I see no reason why it shouldn't work in other situations. We should be able git cherry-pick this across to BLAZE_9184.

@har917 har917 mentioned this pull request Jul 15, 2024
1 task
@AlisonBennett
Copy link

@har917 can I cherry pick this one across to BLAZE_9184 now? I will test it by default when doing the BLAZE 1000pt runs.

@har917
Copy link
Collaborator Author

har917 commented Jul 16, 2024

@har917 can I cherry pick this one across to BLAZE_9184 now? I will test it by default when doing the BLAZE 1000pt runs.

yes - should be ready

@AlisonBennett
Copy link

@har917 I tried the cherry pick but I think that BLAZE_9184 may be too far behind MAIN. A git cherry pick doesn't work because of the change in folder structure, and I have been unsuccessful with the manual cherry pick because the subroutine set_group_output_values in cable_iovars.F90 is not defined in BLAZE_9184 and the function generate_out_write_acc is not used within the write_output subroutine in cable_output_module . The changes made in this commit seem to rely on earlier changes to the output functionality in cable. Can you advise?

@har917
Copy link
Collaborator Author

har917 commented Jul 16, 2024

@har917 I tried the cherry pick but I think that BLAZE_9184 may be too far behind MAIN. A git cherry pick doesn't work because of the change in folder structure.

I did wonder whether this was going to be a problem since I didn't really recognise some of the code in the output section (and it can only go so far automatically) Perhaps the way forward is to manually port the intent of the code across into BLAZE_9184

@AlisonBennett
Copy link

Probably the simplest way forward if we want Pot_Evap in the output :)

@har917
Copy link
Collaborator Author

har917 commented Jul 17, 2024

0a69346 reverts the evaluation of %epot to its original value - this allows %wetfac_cs to be as previous in connections to ACCESS.

Instead a revised evaluation of %epot has been included at line 671 of cable_canopy - scientifically we do not need to weight the vegetation and soil components of potential evaporation by %transd. This weighting has already been included via the net radiation used to determine the component parts.

@har917
Copy link
Collaborator Author

har917 commented Jul 18, 2024

testing of equivalent changes in the BLAZE_9184 branch - see 1f666d3 and 0f297f2 - indicate that this extension leads to bitwise reproducible results on all other variables
Fluxes__Blaze_9184_PotEvap_Diff_acttest9_20240717_164348
Pools_tot__Blaze_9184_PotEvap_Diff_acttest9_20240717_164208
Evap_vs_PotEvap_Blaze_9184_PotEvap_acttest9_20240717_164951

@har917
Copy link
Collaborator Author

har917 commented Jul 18, 2024

*benchcab regression testing
log here
benchmark_cable_qsub.sh.o121035128.txt

This (of course) fails because of the new output variable PotEvap

Somewhat more worryingly - all time varying outputs have also been impacted - see
BR-Sa3_2001-2003_FLUXNET2015_Met_S1_R0_R1_editted.txt for a selection from one site (the full diffs are too large). All these differences are at the numerical precision level - and do not accumulate through time.

Given the equivalent changes applied to the BLAZE_9184 branch produce bitwise equivalence on the other variables I think this has to be something to do with how the new output routine operate.

Sending to review for another opinion as to whether this merits further investigation.

@har917 har917 requested a review from ccarouge July 18, 2024 04:13
@ccarouge ccarouge marked this pull request as ready for review July 19, 2024 04:37
@ccarouge
Copy link
Collaborator

@har917 Looking into it to see where the differences might be coming from so we can exclude the changes here.

Copy link
Collaborator

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

I've looked into the numerical precision differences. It turns out this is linked to the fact we load openmpi in benchcab even when we do serial compilations. We'll need to look into that more in details in benchcab, but it means it's not a problem for this PR to go ahead.

I am approving but I have a question about where the update to epot calculation is placed.

Comment on lines +671 to +673
! INH #335 - we don't need to weight components of %epot by %transd
! however coupled model uses %wetfac_cs so overwrite here before testing in ACCESS
canopy%epot = (canopy%fevw_pot + ssnow%potev/ssnow%cls) * dels/air%rlam
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand your comment here. Are you saying: wetfac_cs is only used in ACCESS model and depends on the epot value. So you don't want to update the epot value in the DO loop before testing in ACCESS model.

Shouldn't it be irrelevant because either we need to weigh epot with %transd or we don't. Shouldn't this be a physics consideration and not a "let's see how the model responds" type of decision?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally we would change %epot and hence %wetfac_cs in the section above this change - and use the correct value in both offline and coupled. However since %wetfac_cs is one of the coupling variables - and one that I think does get used - I think this needs independent testing within ACCESS (and so is a separate issue).

In the interim I opted to adjust %epot after the evaluation of %wetfac_cs which will allow updates to CABLE3 to be ported across to ACCESS as a first step towards resyncing the code bases.

@ccarouge ccarouge merged commit e6be17c into main Aug 6, 2024
7 checks passed
@ccarouge ccarouge deleted the 335-facilitate-output-of-potential-evaporation-directly-from-the-offline-code-base branch August 6, 2024 23:33
@SeanBryan51 SeanBryan51 restored the 335-facilitate-output-of-potential-evaporation-directly-from-the-offline-code-base branch August 8, 2024 02:37
@SeanBryan51
Copy link
Collaborator

Restoring this branch to reproduce CABLE-LSM/benchcab#304.

@SeanBryan51 SeanBryan51 deleted the 335-facilitate-output-of-potential-evaporation-directly-from-the-offline-code-base branch August 19, 2024 00:24
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.

facilitate output of potential evaporation directly from the offline code base
4 participants