search

APSIMInitiative / ApsimX

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

Missing reference variable in canola model #8325

Open byzheng opened 9 months ago

byzheng commented 9 months ago

What is your question?

In the master branch of canola model, there is a reference variable [Phenology].Vegetative.PhotoperiodEffect in several phenology stages.

(https://github.com/APSIMInitiative/ApsimX/blob/a161da1008c2cca23c9f437b8d80feafd6d2db14/Models/Resources/Canola.json#L691)

image

However, this variable doesn't exist in [Phenology].Vegetative.

Should I expect an error message for missing variable?

par456 commented 9 months ago

If I try to look at [Canola].Phenology.LateGrainFilling.Stress in a report, it does indeed throw an error about it not existing. Since it's not throwing an error otherwise though, my guess is the canola model isn't using that variable or trying to calculate it.

Will need someone with a bit more knowledge to say if this is an issue that should be fixed and if so, how it should be fixed.

byzheng commented 9 months ago

From canola model, I personally think we should remove non-exist reference variables for Stress as they are not used in any places. It causes confusing to read model.

BrianCollinss commented 9 months ago

When we play with the model structure, sometimes we remove a reference to a variable to test something, which means it would not be used temporally. Any error thrown in this situation will cause lots of trouble.

hol353 commented 8 months ago

I personally think we should remove non-exist reference variables

Agreed. Will have to be done manually through as I can't think of an easy way to write code to do this automatically as there are lots of functions that aren't referenced by the code but are provided as outputs for the users convenience. e.g. StartFloweringDAS, MaturityDAS.

hut104 commented 7 months ago

Let's remove the unused "Stress" functions from all GenericPhases's in all resource files or simulations (ie we need a converter). Note: this will soon tell us if someone is actually referring to that stress function in their progression function within the GenericPhase.

stale[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had any activity in the last 30 days. It will be closed in one week if no further activity occurs. Thank you for your contributions.