search

APSIMInitiative / ApsimX

ApsimX is the next generation of APSIM
http://www.apsim.info
Other
136 stars 164 forks source link

Root Class Needs Refactoring #5489

Open hut104 opened 4 years ago

hut104 commented 4 years ago

The root class has been under development as part of work on the sorghum model. As a result, several algorithms have been incorporated into the class code rather than adding new functionality through use of functions (to capture differing options).

Furthermore, an optional switch has been added for calculation of a root front (probably for lateral root growth). However this optional switch is used to switch between all options (ie it is a single switch for options used in the sorghum model as far as I can see). For example, if the the root front switch is present, the model appears to use a different N uptake approach.

The root model needs to be completely refactored to remove these sorts of developments. All options should be captured using functions in the Json rather than optional flags within the code. It is not clear that the Sorghum model (or any model using the flag) will operate correctly with multiple root zones given the subtle differences in code (e.g. CalculateWaterSupply method). Finally, all optional properties should be removed or made mandatory.

jbrider commented 4 years ago

@hut104 I thought we had got it to a point at the last sprint where it wasn't holding you up - that we had enabled the functions to be extracted out? Willing to catch up and discuss refactoring either way. My understanding of multiple root zones is poor - you have mentioned previously that we could do skip row using them - is there an example of a skip row setup we could use to test with?

hut104 commented 4 years ago

Yep. We've been able to keep going. But the code really does need to be cleaned up. I see the same calculations occurring throughout the code - never a good sign. There are also flags to allow optional execution of alternate methods etc. These need to be fixed to follow out guidelines for maintainability.

The strip crop examples are in the microclimate and agroforestry examples (e.g. gliricidia) and tests. We will probably use these in the cotton model development.

Neil.

From: Jason notifications@github.com Sent: Wednesday, 5 August 2020 8:56 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] Root Class Needs Refactoring (#5489)

@hut104https://github.com/hut104 I thought we had got it to a point at the last sprint where it wasn't holding you up - that we had enabled the functions to be extracted out? Willing to catch up and discuss refactoring either way. My understanding of multiple root zones is poor - you have mentioned previously that we could do skip row using them - is there an example of a skip row setup we could use to test with?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/APSIMInitiative/ApsimX/issues/5489#issuecomment-668865542, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC2UVWSJRWXSB6MMT6O7FWLR7CGYJANCNFSM4PUBMIIA.