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

Integrate latest DCaPST and Dynamic Tillering changes. #9312

Open
wants to merge 279 commits into
base: master
Choose a base branch
from

Conversation

joesaddigh
Copy link

@joesaddigh joesaddigh commented Sep 12, 2024

This PR addresses some fixes that were made to the Dynamic Tillering routines. It also integrates DCaPST with some tweaks to parameters, as per Alex Wu's instructions.

Working on #7705

This PR also supersedes: #9008

joesaddigh and others added 30 commits December 29, 2022 10:48
…er to DCaPST models. Add stub for replacements.
…defaults, as per spreadsheet. Start adding Daily Comparisons (New Vs Old) to Wheat model using exported data from Apsim Classic.
…ns (New Vs Old) using exported data from Apsim Classic.
… exactly as is from classic scripts (within the Classic software).
…anges the date format for the combined_<Site>.csv. Remove some of the values from the NewVsOld 9 panel graph.
…or everything apart from CSH13R as this is the only "Tall" Cultivar according to the Classic implementation.
… >= rather than > laiTrigger to match the Classic implementation. Refactor to adopt VS suggestions and add a safety check.
…m/joesaddigh/ApsimX into IntegrateApsimClassicDCaPST_Wheat

# Conflicts:
#	Models/DCaPST/DCaPSTModelNG.cs
…-1 simulations (Change from HE7CultivarM35-1r to HE7CultivarM35-1 and HE8CultivarM35-1r to HE8CultivarM35-1)
…son graphs (DCaPSTVsNoRootAdj and OldFixedVsNoDCaPST).
@joesaddigh
Copy link
Author

Just replying to Dean Holzworth's original comment as this PR is a new version of the originally reviewed PR #9008.

2024-09-11 13_26_59-Integrate latest DCaPST and Dynamic Tillering changes  by joesaddigh · Pull Requ

  • I notice in the stats that the sorghum model performance has changed. Does this mean that DCaPST is turned on by default for sorghum simulations? Does this slow down the runtime speed of sorghum?

DCaPST is not turned on by default. You can only enable DCaPST by adding it to the model under the Paddock.
The stats might've changed due to some changes to tillering and the canopy for Sorghum. However, we also removed some experiments that we deemed as unfit for the validation. We also increased the N by allowing certain experiments through the validation that didn't quite make it to Harvest. There were a number that got very very close but didn't reach the harvest stage due to a freeze event.. This has been reviewed by Graeme Hammer.
We are going to profile DCaPST in a separate PR in the near future to try and speed it up and address any bottlenecks.

  • The stats also show that the number of observations for sorghum has changed, in some cases 'n' has reduced.

As explained above :-)

  • There are also a bunch of files in this PR that probably aren't needed. Under DCaPST there is a Sorghum and Wheat directory. Are these needed?
  • There is also an 'old' directory under Tests/Sorghum.

These files have now been removed.

Please let me know if you have any other feedback and we will be happy to address.

Thanks,
Joe

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