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

Update BLAZE in CABLE-POP_TRENDY #392

Open
Tracked by #232
ccarouge opened this issue Sep 6, 2024 · 15 comments
Open
Tracked by #232

Update BLAZE in CABLE-POP_TRENDY #392

ccarouge opened this issue Sep 6, 2024 · 15 comments
Assignees
Labels
priority:medium Medium priority issues to become high priority issues after a release.
Milestone

Comments

@ccarouge
Copy link
Collaborator

ccarouge commented Sep 6, 2024

Update BLAZE into CABLE-POP_TRENDY branch from the NESP2pt9_BLAZE branch.

Needs some clean up of the BLAZE code when merging the new functionality.

@ccarouge ccarouge added the priority:medium Medium priority issues to become high priority issues after a release. label Sep 6, 2024
@ccarouge ccarouge added this to the BIOS3 milestone Sep 6, 2024
@har917
Copy link
Collaborator

har917 commented Oct 11, 2024

There are around 3 elements to this issue

  1. ensuring that the science code of BLAZE is updated to a form which we think is functional
  2. modifiying the output code - two parts (NEP vs NBP and making sure casa_inout is using the correctly sized variables)
  3. figuring out how to deal with the additional gridded inputs that are needed.

Part 3 intersects with the BIOS merge work #281 since two of the inputs are %modis_igbp and %avannmaxfapar which BIOS reads in via the old binary formats.

@har917
Copy link
Collaborator

har917 commented Oct 11, 2024

%modis_igbp is equivalent in concept to a soil ancillary - so could be introduced via the gridinfo file.

%avgannmax_fapar is better viewed as an internal long-run climate variable - i.e. it implicitly depends on the meteorology and vegetation cover/state - but one that applies on the grid cell not at the tile/patch level. It would appear that the original implementation of BLAZE included an evaluation of this variable within the climate% TYPE (i.e. within call_climate) - however for BIOS this was converted to a new input.

@abhaasgoyal For the moment I think that this simplest way forward is to not include these layers in the gridinfo file, create additional BLAZE netCDF input files for them, and then read them in via a new subroutine via a nearest neighbour search (similar to the GFED data read).

@har917
Copy link
Collaborator

har917 commented Oct 16, 2024

@juergenknauer @ccarouge @Whyborn @AlisonBennett a bit of advice please.

I've been using github to look at the differences between NESP_2pt9_BLAZE and a CABLE-POP_TRENDY branch in order to map out (in a bit more detail) what would be needed to update BLAZE (and hence make it available for Australia and global testing under @juergenknauer efficient run environment).

I'm now confused as to the status of the CABLE-POP_TRENDY branches - in that there appears to be code that I/Alison copied/took from CABLE_POP-TRENDY and placed into the NESP branches (e.g. anything to do with %potstemnpp as per draft PR to keep the two branches in sync scientifically that apparently aren't in the github CABLE-POP_TRENDY branch.

My idea originally was to create a development branch off CABLE-POP_TRENDY (or perhaps CABLE-POP_TRENDY_LUC_bug) and bring the BLAZE related code up to date (and then work on rewriting the inputs code sections) ... but these differences lead me to question the status of these branches. So which CABLE-POP_TRENDY branch would be the appropriate branch to start with?

I note there is also a lot of typesetting changes between the NESP and POP_TRENDY branches - with the POP_TRENDY being (I thought) less in line with UKMO coding standards.

@har917
Copy link
Collaborator

har917 commented Oct 16, 2024

Parts of the codebase that need 'merging' to facilitate BLAZE in CABLE-POP_TRENDY (only focusing on the serial version of the code)

  • POP.F90 - interpolate_firemortality()
  • adjust_POP_for_fire.F90
  • casa_cnp.F90 line 1364
  • all the differences in casa_inout.F90
  • cable_common.F90
  • cable_iovars.F90 (for new output)
  • cable_output.F90
  • science core of blaze,f90, blaze_driver.f90 and simfire.f90
  • the namelists

We will also need to update/add routines to read in modis_igbp and AvgAnnMaxFAPAR via netcdf files (rather than the binary formats that BIOS uses)

@juergenknauer
Copy link
Collaborator

@har917 CABLE-POP_TRENDY is the main branch that should contain all the latest science developments that are tested and working. For example, I branched off CABLE-POP_TRENDY_LUC_bug to fix the constant crop fraction issue. After that work was completed (and test runs successful), I merged it back into the 'main' CABLE-POP_TRENDY branch.

I thought it would be a good idea if you could do the same with the fire development.

Is there anything in particular that should or should not be in the CABLE-POP_TRENDY branch?

@har917
Copy link
Collaborator

har917 commented Oct 16, 2024

@juergenknauer If you look at the differences in the draft PR linked above you get a whole stack of differences which don't make sense (given my understanding of the history of the developments). Closer inspection of the actual code indicates that many of these differences are not really true.

However, a bit more digging indicates that my issue arises because that github produces three dot (...) differences (so differences since the last common point of the two branches) by default, not two dot (..) differences. i.e. this is down to me not understanding github

Now forcing github to come up with two dot differences produces an even scarier list - but a step forward at least. However these wide spread differences mean that testing is going to be difficult/near impossible since I can't create known good output from a branch which has all the CABLE-POP_TRENDY science.

@har917
Copy link
Collaborator

har917 commented Oct 16, 2024

In answer to your specific question - we do need to pull the code to output Potential Evaporation across from NESP/MAIN into CABLE-POP_TRENDY at some point.

@Whyborn
Copy link

Whyborn commented Oct 16, 2024

CABLE-POP_TRENDY is definitely the branch that you should branch off. We're treating this as the "main" branch for the POP/LUC developments of the code (can maybe think of it as the 2nd trunk). I can't say much on the "missing" bits of code, other than I think it's probably best not manually pull code across branches. I'm taking a browse through the diffs and it looks like there are some blocks unrelated to BLAZE that are included in NESP2pt9 and not in CABLE-POP_TRENDY. I would lean towards removing this blocks?

Which formatting standards are these? We have some coding standards here, which are based on the JULES standards.

@Whyborn
Copy link

Whyborn commented Oct 16, 2024

@juergenknauer a side note, does that mean the existing CABLE-POP_TRENDY_LUC_bug branch can now be deleted? Or are there still bits there you need to keep?

@har917
Copy link
Collaborator

har917 commented Oct 16, 2024

@Whyborn There is a set of changes necessary to correct and get the Epot diagnostic out of the code that is in NESP/BLAZE_9184 and MAIN (I think) but not in POP_TRENDY.

There are also other changes that have made there way into MAIN which would be useful to have in POP_TRENDY prior to the creation of CABLE4 - e.g. lakes fix, screen Tq fix etc.

@Whyborn
Copy link

Whyborn commented Oct 16, 2024

These changes for the Epot diagnostic sound like changes that should be merged into the POP_TRENDY branch then?

These changes should be achieved by the merging of POP_TRENDY into main, rather than the other way around. If we try to cherry pick changes from main into POP_TRENDY (either through git or manually), I think it will make the eventual merge of the whole branch back into main more difficult. If they are so crucial that the changes can't wait for this major merge, then people can open pull requests to merge these fixes into POP_TRENDY, but it feels like it could be a bit of wasted work.

@juergenknauer
Copy link
Collaborator

@juergenknauer a side note, does that mean the existing CABLE-POP_TRENDY_LUC_bug branch can now be deleted? Or are there still bits there you need to keep?

@Whyborn yes, that branch is not needed any more. I can try and remove it.

@juergenknauer
Copy link
Collaborator

@juergenknauer a side note, does that mean the existing CABLE-POP_TRENDY_LUC_bug branch can now be deleted? Or are there still bits there you need to keep?

@Whyborn yes, that branch is not needed any more. I can try and remove it.

Looks like I have deleted it.

@har917
Copy link
Collaborator

har917 commented Oct 17, 2024

Created development branch cable-pop_trendy_blaze from CABLE-POP_TRENDY for this work.

@har917
Copy link
Collaborator

har917 commented Oct 17, 2024

BLAZE requires additional input files - specifically modis_igbp and AvgAnnMaxFAPAR (and HYDE human population density data).

For the working BIOS-configuration (as per BLAZE_9184 and NESP_2pt9_BLAZE) these data were processed and converted into .bin files. I have developed (hopefully) equivalent input at 0.25 degrees, global in netCDF format and placed in /g/data/x45/Data_BLAZE2/ (alongside copies of the other inputs which are already in .nc format).

NB we need to sort out permissions on some of the directories in /g/data/x45 as they are group read but not group write.

SeanBryan51 added a commit to CABLE-LSM/gists that referenced this issue Oct 18, 2024
These will be incorporated via a separate input file - see
CABLE-LSM/CABLE#392 for more details.
SeanBryan51 added a commit to CABLE-LSM/gists that referenced this issue Oct 18, 2024
These will be incorporated via a separate input file - see
CABLE-LSM/CABLE#392 for more details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:medium Medium priority issues to become high priority issues after a release.
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

4 participants