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

322 use netcdf inputs for bios met #458

Open
wants to merge 9 commits into
base: CABLE-POP_TRENDY
Choose a base branch
from

Conversation

Whyborn
Copy link

@Whyborn Whyborn commented Nov 4, 2024

CABLE

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

Description

This PR routes the BIOS inputs through the new input routines developed for the CRU/TRENDY experiments. It involves some minor restructuring of the existing routines to handle instances where an amount less than the full set of possible variable datasets are passed via the namelist. The cable_user%RunType='bios' still exists for the ancillaries. This should operate completely independently of the ancillary work.

The outputs of the 9 point ACT test are bitwise equivalent with 2 caveats:

  • The handling of the WG%TempMaxDayPrev is corrected in the original BIOS routines (handling at first day of run was incorrect).
  • The longitude/latitude values in the output differ on the order of the floating precision delta, since they are computed in different ways.

I am doing some longer term work in neatening up and improving the met routines to generalise them for more arbitrary sets of input variables, but this serves as an acceptable interim that should work seamlessly with Abhaas' work with the ancillaries.

Part of #(322)

Type of change

Enhancement

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

Involves some restructuring in the SPATIOTEMPORALDATASET type to handle cases where given variables are not given. Work was ported from a working branch that got a bit messy during testing.
Now get bitwise identical results to the POP_TRENDY branch when running the BIOS configuration (with the correction for previous day temperature in the BIOS code)
@Whyborn Whyborn requested review from har917 and ccarouge November 4, 2024 10:25
Copy link
Member

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

It's good. Just a few comments and potentially a mistake.

offline/cable_driver.F90 Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_bios_met_obs_params.F90 Outdated Show resolved Hide resolved
Includes some improved and corrected commentary, removal of magic numbers and made some variable names more self-descriptive. Clarifying date handling workflow to come.
Existing code is commented out as I need to re-run the BIOS test to ensure correctness before removing it entirely.
Copy link
Member

@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 generally like it but a few more comments in. There is a weird one in there which could potentially be a bug, so not approving to make sure this is resolved before it gets merged.

offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Show resolved Hide resolved
offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
Add checks that nearby temporal searches aren't looking too far, minor errors in inequalities and mistake in next day variable.
@Whyborn Whyborn requested a review from ccarouge November 26, 2024 22:33
Copy link
Member

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

The changes from the review are ok although it seems to be missing an END IF. Did it compile?

Also, you have conflicts with CABLE-POP_TRENDY branch to solve now.

offline/cable_cru_TRENDY.F90 Outdated Show resolved Hide resolved
@har917
Copy link
Collaborator

har917 commented Dec 9, 2024

@Whyborn @ccarouge Apologies I've just returned to trawling through github notifications and realised I hadn't looked at this from last month. Could you go through and let me know if there are any unresolved questions from the thread?

In the main I'm less worried about maintaining reproducibility that I am about getting the science correct (but realise that's likely not how the NRI want to work)

@Whyborn
Copy link
Author

Whyborn commented Dec 9, 2024

I'm in the process of merging into POP_TRENDY, just need to test after addressing the merge conflicts. I picked up a mistake I'd made in a prior PR while doing this, so got sidetracked fixing that.

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