APSIMInitiative / ReferencePanel

2 stars 2 forks source link

Guidelines for Development #61

Closed sarahcleary closed 3 years ago

sarahcleary commented 4 years ago

As discussed in the June 2020 RP Strategy Session, the following dot points are the basis for "guidelines for APSIM model development"

• All crops must communicate with MicroClimate, implementing the ICanopy interface. • All crops must communicate with SoilArbitrator, implementing IWaterNItrogenUptake. • All crops must communicate with crop consumers (animals, harvesters), implementing IPlantDamage and IOrganDamage. • Each crop needs to include grazing or pest damage, intercropping, phosphorus and swim examples (lifecycle needs to provide dummy pests and diseases to allow sensibility testing). • All models must be compatible with SoilWater, SWIM, Nutrient and SurfaceOrganicMatter. • Soil water models must provide the necessary variables for soil arbitrator to use. • Appropriate stress responses: Stress Response
Water deficit Mandatory
Water excess Desirable
Nitrogen deficit Mandatory
Phosphorus Mandatory by 2022
CO2 Mandatory

@APSIMInitiative/reference-panel - can you please review/amend etc..

Once revised and accepted, will add this to www.apsim.info

peter-devoil commented 4 years ago

Would be good to have links. Can anyone point me to a IDamage interface, or is that aspirational?

hol353 commented 4 years ago

I have updated the above list of dot points. The inverse of each dot point also holds true e.g. crop consumers must damage plants via the IPlantDamage and IOrganDamage interfaces. Not sure if we want to be this explicit though.

peter-devoil commented 4 years ago

More suggestions for stress response are welcome.

sno036 commented 4 years ago

I'd add that any new soil, organic matter, solute and surface organic matter (any more for this list) must be compatible with MicroClimate and SoilArbitrator using the above interfaces as sensible.

peter-devoil commented 4 years ago

@sno036 , are there interfaces that Microclimate & SoilArbitrator use other than the first two links (ICanopy & IWaterNItrogenUptake)? If not, we can just describe them as "inverse" like dean suggests.

Any more suggestions for the stress response table?

sarahcleary commented 4 years ago

@APSIMInitiative/reference-panel - any further comments? @sno036 - response to @peter-devoil's question above?

sno036 commented 4 years ago

@hol353 would be better probably to add the list of interfaces for Microclimate & SoilArbitrator. The inverse requirement applies as well.

hol353 commented 4 years ago

When a model needs to provide functionality for another model (i.e. respond to requests from another model) it needs to implement these interfaces.

Model type (responding model) Calling model Interface to implement
Plant Canopy interception ICanopy
Plant Soil arbitrator IUptake
Plant Crop consumers IPlantDamage
IOrganDamage
Plant (too general - remove) User interface, stock, other IPlant
Surface organic matter Plant, Stock, SimpleGrazing ISurfaceOrganicMatter
Solute all ISolute
Nutrient Surface organic matter INutrient
Water balance all ISoilWater
Weather all IWeather

The way to read this table e.g. A plant model must implement the ICanopy interface. The inverse of this is that a canopy model must [Link] to an ICanopy interface when it needs to communicate to a plant model.

The fact that the above table doesn't have canopy in the first column indicates that communications between plants and the canopy model are one-way, i.e. canopy calls plant when it needs to and the plant model responds.

There are many other interfaces that models use within them and their child models e.g. within PMF.Plant and the Nutrient models. There are also many other interfaces that the infrastructure provides to let models call into the infrastructure. To keep the above list manageable, I've just listed the important ones for model builders.

I've left phosphorus and swim off this list as the don't fully exist tyet. Will add them when they are released.

For PMF models, the PMF organs GenericOrgan, HIReproductiveOrgan, Leaf, Nodule, PerennialLeaf, ReproductiveOrgan, Root and SimpleLeaf have been used in many released models and the validations and tests of those models have shown they implement the above interfaces correctly. The exception to this are the damage interfaces. This is new work that none of the released models have tests for. When new models come along that only use the above PMF organs then I'm not sure there is much value in the model developer providing additional tests.

Finally, it would be good if this type of documentation went on apsim-dev rather than www.apsim.info so that we keep the model documentation together.

sno036 commented 4 years ago

Good summary thanks @hol353. I think I would add that we would like to see more of the "example" simulations. At present they are mostly (some exceptions) of a type that might be described as "here is a simulation that includes the model - it runs - good luck". TBH - they are mostly not really a good example for new users (of APSIM or that model) to start from or to learn how to use the model.

As the beginnings of an example of a better example (I couldn't resist) @mpandreucci is putting some effort into a better example for AgPasture. There is a plain-jane simulation but there are also a few experiments of the type users might want to do, there are manager examples that might be of assistance, the functionality in Report is used (rather than just a daily report of variables).

A 'formula' for an example for a crop model might be something like:

  1. Short memo describing the model and example
  2. Plain-jane simulation but with decent managers for the crop type and some effort in the Report model showing main outputs
  3. The crop in a rotation with another (good example of sowing and harvesting rules)
  4. The crop intercropped with something else (include some good managers and reporting examples)
  5. The crop being eaten by stock (using SimpleGrazing probably and sensible physiological stages) or something similar
  6. An irrigation x fertiliser experiment or something relevant
  7. Extra points for a special zone type thing (see Gliricidia)
  8. When it comes along, the crop being damaged by something from LifeCycle
  9. When SWIM is released, a SoilWat v SWIM simulation

I know this seems to be putting a lot on the crop examples but you croppy guys think that crops are the main thing don't ya? Also, if there were one good/full/leave-place-holders-for-models-above-not-yet-there then it would both give a good demonstration of what is wanted/expected but would also make it relatively easy to construct an example for a new crop.

My $0.02 worth anyway.

sarahcleary commented 4 years ago

Notes from July RP meeting: Discussion around whether retrospective review of models should occur. Agreed that this would be preferable and encouraged but noted availability/capacity to do this, may not be feasible for all models. Mention made that without consistency, it could be seen that APSIM is a number of disparate applications under a single banner. Retrospective review will reduce these inconsistencies. Noted that CLEM won't comply due to the monthly time steps. Guidelines are being set by the @APSIMInitiative/reference-panel and new modules will be required adhere unless there is a valid reason for the module not to comply. An example would be MicroMet which would not require to interact with solutes.

Discussion on examples - what should be expected? Agreed that there should be more effort put into example simulations, especially to assist with training new users to the model. Reduces the learning curve with utilising a new model. Noted that sometimes data isn't available. Agreed that examples are beneficial as it demonstrates functionality. If it isn't 'real' - need to ensure documentation is clear as users may assume validity.

(1) There should be an equivalent of iSow, iKill, iDamage, iHarvest - add to required implementation list.
(2) Include a generic management component to utilise this feature.

ACTION Include a few more required interfaces - sowing and killing the crop. Question is to how to generalise this across all plants.

ACTION @hol353 to finalise with input from @APSIMInitiative/reference-panel if required.

hol353 commented 4 years ago

@APSIMInitiative/reference-panel, @hut104 :

I have made a first draft of some updated guidelines for model developers and also a revamped 'reviewers report' that matches the new guidelines. Both are in the attached document. I'm conscious of the process not being too onerous on model developers and reviewers. Hopefully I've got the balance right. Most of the material in the attached document comes from your comments above.

The guidelines are split into software guidelines (source code interfaces) and science guidelines (tests, documentation, examples). When we send out a submission for review we could just ask the reviewer for a 'science review' because most reviewers won't know anything about code interfaces. When a submission also has software changes, we could also ask someone for a formal 'software review' to examine the software aspects and how it is implemented. Some submissions will require both a software and science review. Others only a science review.

Please let me know what you think by posting your comments back to this issue.

Guidelines for development.docx

hut104 commented 4 years ago

What about the following? 1) For source code - adherance to APSIM software style guidelines (e.g. case requirements, not use of regions) and programming approaches (restrictions on some OO approaches, use of interfaces, PMF approach to design, unit tests where relevant). A reference could be made to a document containing these guidelines.

2) For PMF- root models must allow multi-point roots, photosynthesis etc must provide CO2 impacts, must be able to work with microclimate for transpiration

There is a statement about must talk to structure - this may not be required as many crop models are being redesigned to not require this.

I'd suggest that memos in examples should be restricted to the root node (ie not within the simulation as these simulations are used as starting points for work - the memos will become misleading). Memos are great in tutorials, but not so much in examples.

I can perhaps see the benefit of testing for rotations. However, for many plants these simulations are quite artificial or uncommon. The same is true for damage tests where there is no idea what the impact of damage should be (ie no data exists) I guess these could be put into the test file (with validation and sensibility tests) but excluded from the documentation. They do not fit with the concept of sensibility tests which assumes that we know what is sensible (ie use of soft data in comparison to the model). We could have a test for damage and simply test that damage changes the plant. That does not demonstrate that the result is correct, that the software is operating correctly, or that it should be used. However, it does test that the model is responding to a stimulus, which is all we can do until further testing comes along. We used to call these refsets or so on (ie just to check that the model responded). I don't think we want these appearing in documentation.


From: Dean Holzworth notifications@github.com Sent: Monday, 17 August 2020 2:24 PM To: APSIMInitiative/ReferencePanel ReferencePanel@noreply.github.com Cc: Huth, Neil (A&F, Toowoomba) Neil.Huth@csiro.au; Mention mention@noreply.github.com Subject: Re: [APSIMInitiative/ReferencePanel] Guidelines for Development (#61)

@APSIMInitiative/reference-panelhttps://github.com/orgs/APSIMInitiative/teams/reference-panel, @hut104https://github.com/hut104 :

I have made a first draft of some updated guidelines for model developers and also a revamped 'reviewers report' that matches the new guidelines. Both are in the attached document. I'm conscious of the process not being too onerous on model developers and reviewers. Hopefully I've got the balance right. Most of the material in the attached document comes from your comments above.

The guidelines are split into software guidelines (source code interfaces) and science guidelines (tests, documentation, examples). When we send out a submission for review we could just ask the reviewer for a 'science review' because most reviewers won't know anything about code interfaces. When a submission also has software changes, we could also ask someone for a formal 'software review' to examine the software aspects and how it is implemented. Some submissions will require both a software and science review. Others only a science review.

Please let me know what you think by posting your comments back to this issue.

Guidelines for development.docxhttps://github.com/APSIMInitiative/ReferencePanel/files/5082144/Guidelines.for.development.docx

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

EnliWang commented 4 years ago

Some quick thoughts:

Software guidelines:

1) I guess there is a general documentation for those interfaces IPlant, ICanopy, IUptake etc. or they are self- explanatory. Sorry I have been update with coding.

2) A reference to a document containing the above and programming guidelines would help.

3) Would it also be helpful if we have an update re the above at reference panel meeting? This can be high level – just let people like me gain some quick knowledge?

Science guidelines:

1) Test/validation – should we have some requirement on the data, i.e., observed data to test the model should cover at least xxx number of contracting cultivars and environments. These can be representative cultivars from different regions. Contrasting environments can be different treatments or sites with contrasting climate and soils.

2) Sensibility test – the sensibility test can expend the cultivars and environments of the observed data, in particular for GxExM situations that are thought to be important, but we don’t have data yet.

3) Documentation – It should be mandatory for any variable/equation in the documentation to have unit. I get very confused/frustrated every time when I try to see whether an equation is right without knowing the units of the variables in it.

4) Examples – I feel to have those examples is very good. Even many are not real, they help people on how to do different things. Many can be shown in the factorial example as ‘experiments’. An emphasis to include GxExM would make such a factorial example extremely useful.

sarahcleary commented 4 years ago

@APSIMInitiative/reference-panel - any further comments?

jbrider commented 4 years ago

@hol353 the word document has a lot of information in it but is much less readable than the table structure we started with at the top of this issue.
Where is this documentation going to reside - you mentioned apsim-dev in the initial post - is that within the development section on the netlify site? Could this be part of the initial readme on the github site?

hol353 commented 4 years ago

Yes I had trouble writing the Word doc and was unsure whether a simple table is better than a description of it. The problem with the table by itself is that it isn't precise enough. I might have another go at redoing the table to cover more scenarios. I think a lot of the problem goes away if we just have better documentation describing each interface and then an overall landing page for developers that guides the reader to the different interface descriptions depending on what they are trying to do.

The developer guidelines should probably be a page on the apsimdev.apsim.info site and pointed to via a page on www.apsim.info. Yes, the readme on GitHub should point to the developer guidelines.

jbrider commented 4 years ago

Definitely missing an initial landing page that documents the different interfaces. Documenting the individual interfaces would be good - and possibly address a lot of the issue. I agree that some of the interfaces will need more description just that doesn't fit into a single table. I don't see anything at apsimdev.apsim.info - is that just the location you think would be a good place for dev documentation?

hol353 commented 4 years ago

There is nothing at apsimdev yet. Needs to be added.

hol430 commented 4 years ago

There is stuff at apsimdev (link below). You can add an article as a markdown file if you want, by committing to ApsimX/Docs/docfx/articles/

http://apsimdev.apsim.info/apsimx-docs/

hol353 commented 4 years ago

@APSIMInitiative/reference-panel: In response to your comments above, I've done another iteration of these guidelines. The guidelines are now part of the APSIM Next Gen documentation web site here:

https://apsimnextgeneration.netlify.app/development/

I have separated the guidelines into science guidelines and software guidelines. When the RP sends out a submission for review, we want the reviewer to look at the science guidelines. I've also updated the form / instructions we send the reviewer: Reviewer form.docx If you have any suggestions please let me know.

jbrider commented 4 years ago

Looks good. In the code examples the comments are easier to read than the code itself - the code is a gray text.

hol353 commented 4 years ago

Hmmm must be something to do with the css styling of the netlify site. @hol430 - any suggestions how we can improve this?

hol430 commented 4 years ago

The colours in the code blocks are all very light, they look like they're intended to be used against a dark background. The easiest solution is probably to enable the dark background. Otherwise someone will need to go through every colour used for those code blocks (there are a lot).

sno036 commented 4 years ago

@hol430 - I know this is just an excuse for you to move everything to your preferred dark background! Perhaps unrelated but the documentation site is very sad at present!

hol430 commented 4 years ago

Well I don't really mind if the default is dark or not, since I have a browser extension which makes it look dark for me anyway. If you guys prefer light, then I'm happy for that to be the default. As per linked issue - it's just a matter of choosing a different theme/colour scheme.

peter-devoil commented 4 years ago

The software interface table is a great leap forward. My only complaint is that I feel it's missing a preamble (landing page?) that describes why this set of interfaces has been chosen - an outline of the problem domain that we expect modules to operate in. We have models that don't operate in this space (clem, oilpalms etc), and some concrete examples of what this space is (eg rotations, intercrops, edible) will justify the list of demands.

sarahcleary commented 4 years ago

Should this landing page be linked or on this one: https://www.apsim.info/improvements/

hol353 commented 4 years ago

@peter-devoil : I think the plan is that all models (except CLEM - which isn't really part of APSIM) follow the guidelines. OilPalm does I think. We probably need to do a bit of a check which ones don't. So there isn't really a 'space' that this applies to. The interfaces came about via evolution (like everything in APSIM). Some of them originally came from APSIM 7.10 and have since been enhanced. Others are new to Next Gen. i'm not sure what a landing page could say.

@sarahcleary : yes, we should link to the page https://apsimnextgeneration.netlify.app/development/ from www.apsim.info. Perhaps say something like, 'If you are thinking of contributing to APSIM then please read the development guidelines'.

EDIT: @jbrider : I have changed the styling slightly to improve readability of code.

@sno036 : What is 'sad' about the documentation site? Any concrete things we could do to improve it?

hol430 commented 4 years ago

Well the software interfaces page talks about models but it doesn't actually specify what a model is. It also doesn't mention the IModel interface which to me is the most important one by far (although to be fair I'm not a model developer). On a somewhat-related note, I wonder if the other interfaces should all inherit from IModel? It would mean that you can't implement, say, IUptake without also inheriting Model but we don't actually do this anywhere currently, and if we did I somehow doubt it would work. There are lots of casts to IModel all over the place.

sarahcleary commented 4 years ago

@hol353 - https://www.apsim.info/improvements/ updated.
News article?

hol353 commented 3 years ago

How about this for a news article:

APSIM is open source for non-commercial R&D use and we welcome all changes and new submissions to APSIM. If you're thinking about making a change to APSIM or building a new model for APSIM then you need to read the page here: https://www.apsim.info/improvements/. We have recently upgraded the development guidelines to make it clearer what we expect when you submit a change to APSIM. If you have questions please do so via our GitHub repositories: https://github.com/APSIMInitiative/ApsimX for APSIM Next Generation or https://github.com/APSIMInitiative/APSIMClassic for APSIM 7.10

sarahcleary commented 3 years ago

Closing Issue. https://www.apsim.info/guidelines-for-development-of-apsim/