Closed hut104 closed 3 years ago
@yashvirchauhan - please raise your question in relation to difference between this new model and APSIM Classic Soybean model within this issue.
I would like if the Next gen developers of soybean and chickpea models could enlighten me in what respect the science in the apsim nextgen soybean and chickpea from the APSIM classic models? Thanks.
Hi, Firstly, note that the NG chickpea and soybean models are not yet released and so the science is not as yet settled. Having said that, much of the basic approach is similar in that we use RUE for photosynthesis, similar approaches for thermal time, etc However the implementations are slightly different and so many parameters have different meanings and so need to be altered. A more significant difference is in the calculation of transpiration - the new models only support penman monteith via the microclimate model (this was optional in apsim classic). The new legume models have a much more mechanistic approach to N fixation and partitioning within the plant. The arbitration rules are therefore also different to APSIM classic (see recent paper by Brown et al on the APSIM NG plant arbitrator). Uptake of soil water is still via KL, and uptake of N follows the approach from APSIM classic. The phenology models are basically the same, but extra stages have been incorporated into the model to match the VR system used in soybean etc.
Thanks Neil There are a few additional issues in both chickpea and soybean models that I think that may need to be fixed for the APSIM NextGen. In chickpea I have found solution to poor prediction of various phenological events especially flowering time. Had a brief presentation at the todays reference panel meeting on this aspect. As a new team in DAF has been formed on crop improvement modelling, will be happy to chat with you guys to keep ourselves abreast with the latest developments and contribute to your efforts where you think we could e.g. on chickpea phenology.
@sarchontoulis - FYI
@sarahcleary This model should now be ready for review. Soterios and I suggest Enli Wang as a potential reviewer given his recent experience in Soybean modelling with a student in China.
@APSIMInitiative/reference-panel - please note that this model is ready for review. @EnliWang - are you willing to review this model? @APSIMInitiative/reference-panel - anyone else that should be nominated as a reviewer?
@yashvirchauhan - I believe this falls within your area of expertise. Would you mind providing a review for this model?
Thanks Sarah. Could you please wait until I attend the APSIM week so that I can evalutate it with the soybean data I have.
discussion in the next RP meeting? @yashvirchauhan - maybe you can seek assistance from RP NextGen developers/users?
@EnliWang and @yashvirchauhan - as discussed in today's RP meeting, this model is now ready for review. As you are aware, the review process is now conducted solely via GitHub. You will note the checklist can now be found here: https://www.apsim.info/improvements/submission-of-improvements/ under "What the reviewer will be looking for". You should be able to find all of the modified files here: https://apsimnextgeneration.netlify.com/development/contribute/ If you have any questions, please let me or @peter-devoil know in first instance.
Review by @EnliWang: @hut104 & @sarchontoulis - I have asked Yushan Wu (the former PhD student) to test the model with her data from two sites in China. I have attached a Soybean.rar file, which contains the modified Soybean.apsimx and Observed.xlsx, together with the met files for the two Chinese sites. The Yaan site provides data for 3 cultivars (early-, mid-, late-maturing) in three years (2014-16), while the Heze site for one (different) cultivar in three years (2014-16). The simulations results are not bad, but we needed to twist the AreaLargestLeaf and PotentialHarvestIndex a bit. However, the additions seems to upset the graphs under the ‘Combined Results’. I could not work out why, hope you can find what’s wrong and fix it.
Can't upload the .rar file to GitHub. Any ideas?
Convert to another format (e.g. .zip). I can do this if you send it to me.
@hol4300 sent, it's 19MB
@yashvirchauhan - are you able provide an update on your review? if you have any issues, please let us know
Hi, Have the module developers tested that all the variable that can be reported actually are actually able to output those variables. I could not output flowering and harvesting dates. it says 'Error in report HarvestReport: Invalid report variables found: Soybean.Flowering'. This suggests that each and every variable need to be tested. This is also an issue occasionally experienced in APSIM classic.
Hi @yashvirchauhan - I'm not sure where your Soybean.Flowering
variable came from, but as you've discovered, it's not a valid variable, and I can't see any documentation saying that it is.
If you open the latest version of the soybean test set, the harvest report already contains variables for flowering and harvesting dates, which are [Soybean].Phenology.FloweringDAS
and [Soybean].Phenology.MaturityDAS
respectively.
Thanks, am I missing some component of the model? It did not work for me as you suggested. Probably I will have to drag your example and see how it works. I am new to NextG and hence struggling.
Hi Yash, Try [Soybean].Phenology.EmergenceDAS [Soybean].Phenology.FloweringDAS [Soybean].Phenology.MaturityDAS
From: Yash Chauhan notifications@github.com Sent: Friday, 29 May 2020 11:47 AM To: APSIMInitiative/ApsimX ApsimX@noreply.github.com Cc: Huth, Neil (A&F, Toowoomba) Neil.Huth@csiro.au; Mention mention@noreply.github.com Subject: Re: [APSIMInitiative/ApsimX] New Soybean Model (#2731)
Hi, Have the module developers tested that all the variable that can be reported actually are actually able to output those variables. I could not output flowering and harvesting dates. it says 'Error in report HarvestReport: Invalid report variables found: Soybean.Flowering'. This suggests that each and every variable need to be tested. This is also an issue occasionally experienced in APSIM classic.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/APSIMInitiative/ApsimX/issues/2731#issuecomment-635709045, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC2UVWV6IO6K5QOOE3VMTKLRT4H2DANCNFSM4FHZPZRQ.
Hmm, what went wrong exactly? Can you provide an error message? If you're referring to the above error about Soybean.Flowering
, it just means you need to remove Soybean.Flowering
from the harvest report. To do this, you can click on the harvest report, go to the variables tab, and remove the line which says Soybean.Flowering
.
Ok, [Soybean].flowering did not work but as suggested [Soybean].Phenology.FloweringDAS worked fine. I instead of copying typed and added a . between flowering and DAS that caused it to crash. The error it was throwing earlier was 'Error in report HarvestReport: Invalid report variables found:. But I wonder why it could not output that? If it is there, it should generate some output?
Which version of apsim are you using (it should say at the top of the window)?
As for why you were getting the earlier error about invalid variables - typing in Soybean.Flowering
would be just like typing in asdf
. Apsim just doesn't understand it, and in that scenario, it will throw a fatal error.
Would download the latest and work with that. When it can pick grain weight or crop type from the same source, what prevents it picking flowering or harvesting?
Drew, where do I find the test run the soybean test set. Can you provide the link?
Hi Yash, yep you can find it here. Just click on soybean.apsimx, then click download. You might need to download the .met files too if you don't already have them. Unfortunately I can't see a way to download the whole folder at once though so it might take a bit of clicking...
As for your previous question, the only reason Soybean.Flowering
doesn't exist is because no one has added it, presumably because it's already accessible as Soybean.Phenology.FloweringDAS
. Generally in next gen there aren't too many variables exposed at the plant level (ie Soybean.X), and instead they're exposed as properties/outputs of the sub-model, in this case phenology.
Sorry Drew, can not save the downloaded file. I do not know how I did it first time? It just open a kind of html page.
Hmm, you might have to do ctrl+s, or right click -> save as, depending on your browser.
@yashvirchauhan - any update with your review of this model?
@yashvirchauhan - any update with your review of this model?
Sorry Sarah, Working on it. Will submit the review soon. Yash Chauhan
Review of Soybean model in APSIM Next Gen Reviewer Name: Dr Yash Chauhan Does the submission meet the ‘Science Guidelines? Please identify where the submission meets the guidelines and where there are areas for improvement.
Yes. I have tested the model in two environments with two sowing dates each and seems to simulate phenology and yield well. My suggestion is that all released Australian cultivars should be parameterised and included in the list. Could find only Bunya and Soya 971 for which I evaluated the model performance. There should be a way of identifying Australian cultivars or cultivars or other origin.
How do you rate the importance of this submission? Major importance, Important, Limited value.
Major importance. Worldwide soybean cultivation is expanding at a faster rate than any other legume and since soybean is increasingly being introduced in new areas, applications of model will be very valuable in those areas in addition to applications in the existing areas.
How do you rate the quality of this submission? Excellent, Strong, Competent, Weak Excellent.
Is the submission adequately referenced? Yes. What is your overall recommendation? Ready for incorporation into APSIM, ready after minor changes, requires major changes and rereview, unsuitable for incorporation.
The model is ready for incorporation into APSIM NextG. I suggest no major or minor changes except for more Australian released cultivars to be parameterised and included and some way of identifying them (e.g. names in capital) or grouping them separately should be found.
Based on the two reviews of this model I suggest it is ready for release.
No objection from me.
@APSIMInitiative/reference-panel - can you please confirm this is ready for release? Require sign off from CSIRO, ISU, AgR
A yes from me. I reviewed the soybean model.
A yes from me as well as suggested in my review.
Sorry - it is the documentation police here! The "Descriptions" (Dean can assist with the detail of what/where is actually needed) needs the units of the properties inserted so that they show up on the UI for users and in the documentation at https://apsimdev.apsim.info/ApsimX/Releases/2020.10.16.5740/Soybean.description.pdf. This needs to be done before acceptance.
In the documentation, is "5.3 TimeOfSowing" the sensibility analysis? If so then suggest that another location be added where, based on physiological knowledge, a different pattern would be expected. I do want to see this before it is accepted as this type of thing has really exposed some issues in the past and is not a lot of work.
The example is minimalistic but not out of step with most other models. This can be improved over time.
@hut104 Happy with the model but ran into a few minor graph issues (other than the password protected data).
The online documentation is missing most of the Model description section (Section 3). Something must have been toggled recently as Enli's review mentions sections not in the current online documentation. Or maybe it didn't get generated correctly.
I had a thorough look through the graphs - I have assumed the missing observed data was meant to be there if you added the series for it.
Australia->Gatton->Graphs->Biomass->HarvestIndex is blank. Australia->Gatton->Graphs->Canopy->SpecificLeafArea is showing observed but not predicted. Australia->Katherine->Katherine1988->Canopy->LAI is not showing observed data. Australia->Kununurra->Kununurra1980->Biomass->BiomassN is not showing observed data. Australia->Kununurra->Kununurra1980->Canopy->LAI is not showing observed data. Australia->Kununurra->Kununurra1979->Biomass->BiomassN is not showing observed data. China->Heze->Graphs->Biomass->LeafLiveWt is not showing observed data China->Heze->Graphs->Biomass->StemWt is not showing observed data China->Heze->Graphs->Biomass->PodWt is not showing observed data China->Heze->Graphs->Biomass->HarvestIndex is blank
Combined Results ->Evapotranspiration is missing table 'PredictedObservedET'
@hut104 - this is now with you?
@APSIMInitiative/reference-panel
This model was submitted for review on 5 March with suggested reviewers. The review process completing on October 15 with further input from the RP after this date with more suggestions. It is worth noting that funding for this development within CSIRO was provided in the previous financial year. I am now funded for work on other crops with contractual milestones in the coming months. I suggest that review processes should take into account working committments of individual developers.
It may also be worthwile thinking about the best way to capture feedback. In this case the feedback was provided in a mix of pdf, docx, github comments, etc.
There is great feedback in the reviews and input from the reference panel. I have summarised it below. I feel that some of this is directly related to soybean model development and some are more generic requirements of APSIM. I suggest that we finalise the work directly related to soybean, and that the other work be assigned to various developers for addressing. Whilst useful and important, the many general improvements will require significant time. Furthermore, many of these changes would be best addressed in a software sprint and some would best be addressed in a refactoring of classes to either remove the problem to provide a more general solution. I will seek to find somebody who can take on the soybean changes. I suggest the RP consider how to resource the general work.
Issues related purely to the soybean model
Reviewer one suggested some changes specific to the soybean model.
Rename EndGrainFill to EndGrainFilling for consistency Agreed.
Reference for cardinal temperatures for vegetative and reproductive Thermal Time Agreed.
memo for EmergenceDAS has "maturity" instead of "emergence" Agreed
explanation for why all leaf mass is assumed to be structural The same was assumed in the original crop models in APSIM. No change.
Request for references for (Min, Crit,Max) N conc values for all organs. These values have been fitted to the available data. No change.
Note that retransfraction fraction is 0.5 even though all leaf is structural Agreed. Though this will have no effect, the value should be zero to avoid confusion.
Reference for RUE This value has been fitted to available data. We could compare this to other models/measurements but the value in the model is for the whole plant (tops + roots) and so relies on parameters used for below ground partitioning. Perhaps we just say this.
Reference for temperature effect on RUE Agreed.
The reviewer questioned whether the function "Rate" was really a rate.
Unsure what is required here. We have many rate coefficients with values of per day.
Sugggested changing the leaf detachment rate to a thermal time basis rather than a daily rate. There no data on detachment rates and so a simple value was chosen to ensure that leaves were not retained in the canopy. No change.
Questioned why Leaf and Grain biomass removal for Prune and Graze had defaults of zero. Agreed. Suggest removing all the defaults except for Harvest and Cut for all organs.
Noted the small range between maximum and minimum NConc These values are simply derived from the data we had available to us. No change.
Requested documentation value of DMConversionEfficiency for Grain Agreed
Question as to why Soybean does not take up NH4 Agreed. We could explore enabling this as a simplest way forward. Otherwise, leave this as is for now (as was the case in APSIM classic) if model fit is impacted.
Need to explain why Root and Nodule CriticalNConc is set to MinimumNConc Agreed.
Units for MaximumFixationRate Agreed. will need units in Json to be added.
Explanation for default biomassremovals for Root and Nodule Agreed. Though they are estimates only with no data behind them (ie to simply have some root senescence in response to damage)
Question as to why no stem N demand during grain filling We see no evidence of stem N storage during grainfilling. There is no stem structural growth during this time. No change.
Reviewer 1 also provided good quality data from china to extend the model testing.
Agreed. These efforts were very much appreciated and the tests have been already incorporated into the model test set.
Reviewer 1 highlighted areas for model improvement (1) leaf number vs leaf size, 2) response to water stress, 3) grain growth and harvest grain yield) Agreed. However there is currently no resourcing to take model development further prior to release. I suggest that the modelling community address this after release with further testing and datasets as they move forward.
Validation datasets: The description of each experiment(s) can be improved, it is best to use small sections/dot points to describe: a. Study site, soil and climate b. Treatments c. Crop & soil measurements d. Simulation validation – what are compared e. Potentially - a summary of model performance: what’s good, what needs improvement Disagree. A simple sentence on each of the above information and a relevant citation is probably all that is required rather than long text. A list of is what is compared is probably not required as the graphs show this. A description of model performance cannot be included because this will change over time. Autodoc cannot know when the behaviour has changed and change the text accordingly.
Key cultivar differences: maybe good to highlight how cultivars differ in terms of phenology parameters These are already captured in the cultivar autodoc sections. Adding more text can be problematic if parameters change without documentation following suit. No change.
deltaLAI not documented. Agreed. Something seems strange here if this important part of the model didn't get picked up by Autodoc
Shell Harvest Index requires explanation Agreed. Memo in shell should explain how an HI approach is used here and why.
Reviewer 2 suggested adding Australian cultivars.
There are already some Australian cultivars within the model as used in the Australian simulation tests. However, we would not have parameters for the latest cultivars. I recommend that this is addressed by model users if/when the model is used in Australia. There is no current resourcing for undertaking this currently.
Reference panel requested checking of empty graphs Agreed. It is not clear why there may be blank graphs now. However, anything could have happened in the 7 months whilst this was under review. Little to no changes were made to the soybean files. Again, it is difficult to maintain such files on a branch for extended periods of time and expect things to continue to operate correctly.
Reference panel suggested extra tests. In the documentation, is "5.3 TimeOfSowing" the sensibility analysis? If so then suggest that another location be added where, based on physiological knowledge, a different pattern would be expected. I do want to see this before it is accepted as this type of thing has really exposed some issues in the past and is not a lot of work. Disagree. There are over a dozen time of sowing experiments in the test set already. The sensibility test for Australia was simply to fill an obvious gap.
General APSIM Improvements
Reviewer 1 also highlighted some issues that are common for other crops, or might impact on the documentation for other/all crops.
Requested changes to Root Class Metadata description to explain a) the meaning of XF b) equation for root front velocity (including temperature effect) c) ROI approach for generalised equimarginal criterion to be explained. d) clarity about N above min N conc e) explanation for kNO3 and kNH4 values f) better comments and units for TemperatureEffect, MaxDailyNUptake, SenescenceRate,RootFrontVelocity Agreed. These will benefit all crop models, and so needs to be worded in a general way only within Root.cs. Other doco should be checked for impacts in other crops.
The leaf class has a fuction called "Tallness" - the reviewer enquired as to whether this is height. Agreed Totally. This should be removed as it duplicates other properties, is confusing and the name is not ideal.
Need better explanation for Leaf and Shell (and others) N retranslocation on non-structural N in leaf class meta data Agreed. The meta data in the code for these properties is too brief. Should look at the metadata for all these arbitration functions. This will impact all crops.
units for initial leaf mass Agreed. I think this is defined in the code for the leaf class. Other units missing in SimpleLeaf.cs should be added as part of these efforts.
leaf storage N demand says "storage biomass" rather than "storage N" Agreed. The metadata in StorageNDemandFunction class needs correction.
Enhance documentation in photosynthesis class metadata to explain how potential RUE is affected by stress. Agreed. Metadata in code should be enhanced, but in a general way to suit all crop models. ie leaving crop specific text to autodoc of functions themselves.
Need some explanation for [Leaf].Fn and [Leaf].Fw in Leaf Class Agreed. May require changes to Leaf.cs, Simpleleaf.cs, PerennialLeaf.cs and SorghumLeaf.cs as they all do something similar.
Description of [Root].RWC Agreed. Root.cs should be checked for better metadata such as this.
Autodoc equation runs of the side of the page Agreed. This is a defect in Autodoc and so should be raised as an issue.
MetaData comment for generic organ states that it is for organs that do not have specific functions. Reviewer suggests that these organs do have a function. Agreed. metadata should be adapted.
For all variables and axis titles of graphs, where needed, a unit needs to be given: e.g. Target =45?, InitialWt =1?. With unit, everything becomes clearer and easier to understand. Possibly. However, this will be a significant effort. There may be impacts on readability of graphs. Options should be investigated.
The map was incorrect and was not represented correctly Agreed. There is a similar issue with all autodoc maps
MaintenanceRespirationFunction: this is set to 1 for root, but not specified for other organs. I am a bit confused about what it does, so some explanation please. Agreed. This is a strange part of the model which seems optional. This will be all through our autodoc. Not sure what to do but suggest a unified approach. This may refactoring the respiration bits in all organs.
FCO2 – it is canot we put this in the user interface to make it clear This is implemented in code for efficiency reasons. Could look into changing metadata in relevant functions. This would improve things for all models.
Leaf model 1) fx CO2internal – should be changed to ‘CO2CompensationPoint’ (ppm)? 2) StomatalConductanceCO2Modifier = (C350+2)/(Ca+2)? 3) Photosynthesis – shouldn’t it be a multiplication function – with FT, FN, FVPD, FCO2 etc and RadnInt? 4) VegetativeThermalTime – XY – why not Xvalu = ?, how was mean air temperature assigned? There are a range of issues here which may be reflected in all other crop models. Some (e.g. 4) have been resolved in the 7 months whilst this was under review. Not sure how to respond.
Members of the reference panel have also suggested some changes
There has been mention in the past of a sprint focused on getting auto doc tidied up. We need to get onto this
@hut104 The reference panel agrees with your proposal that the elements that are general can be assigned to someone else. Your responses to the reviewers and reference panel members have been accepted. There were a few points that you agreed needed to be changed within the soybean model - once those are complete can you let us know and then we will be able to recommend the model for release.
I note the outstanding issue as per https://github.com/APSIMInitiative/ReferencePanel/issues/79 -- Outstanding issue around the lack of units. Model can't proceed to release until issue is resolved or there is a plan/issue to resolve. @sno036 to start an issue/discuss with @hut104 and report back. @sno036/@hut104 - has this been resolved?
Hi all, In response to https://github.com/APSIMInitiative/ApsimX/issues/2731#issuecomment-737664882, we have spent time as part of a documentation sprint to fix generic issues with Autodoc. We have also updated units in the soybean model where we could. Our collaborators from ISU have also added text to documentation memos in response to questions from the reviewers. Code has been updated to address other issues (see above). Can we now 1) move the soybean model into a resource, 2) move the example to the examples folder, and 3) Move the validation set to the test folder, so that the model can be released?
Thanks @hut104. @hol353 The Soybean model can be moved into release.
A new soybean model is to be developed within APSIM Next Generation in collaboration with Iowa State University.