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

Clean up sowing procedure #7008

Closed
peter-devoil opened this issue Dec 14, 2021 · 8 comments · Fixed by #7362
Closed

Clean up sowing procedure #7008

peter-devoil opened this issue Dec 14, 2021 · 8 comments · Fixed by #7362
Labels

Comments

@peter-devoil
Copy link
Contributor

Notes from the sorghum review

  • Fertile tillers are called buds when they're really tillers. There's too much doubling up on parameter names.

  • Units (mm vs m) for row width are not displayed in the summary message

  • Skip row codes should be replaced by text descriptors (eg single, solid etc)

@hut104
Copy link
Contributor

hut104 commented Dec 14, 2021 via email

@peter-devoil
Copy link
Contributor Author

Is there a multi-zone demo that's close to this in there somewhere? We could use that as a discussion start point

@jbrider
Copy link
Contributor

jbrider commented Dec 14, 2021

@hut104 At the moment the skiprow configuration can be configured with a simple script change for default scenarios - which I think is quite useful. I'm probably not biased at all given sorghum is one of the few in the 10% that do use it. I confess I have no idea how difficult it is to setup the multi-zone - but if it's not easy then it becomes a significant barrier. I agree with @peter-devoil that a good example of how it would work would be a good place to start.
Perhaps a tool to define/create the zones might be needed if it is difficult?

I'm not sure what definition of data in the sow type you are referring to though - an option to define the row configuration seems ok as it is very similar to row width (and related). I agree that it shouldn't need extra information in there beyond that - the few crops that use it can look after the calculations given the row configuration. There is too much in there at the moment.

@peter-devoil
Copy link
Contributor Author

Perhaps we're too hung up on typed languages and can pass parameters via a simple dictionary, combined with some helper routines to extract whatever the particular PMF instance wants: buds for vines, FTNs for sorghum, plants_pm for cotton etc.

This would also allow us to correctly detect optional values, instead of guessing from the default (zero) value.

@hol353
Copy link
Contributor

hol353 commented Dec 14, 2021

Using a dictionary will be ugly for the user writing manager scripts e.g.

    wheat.Sow(new Dictionary<string, string()
        {
            {"cultivar", "hartog" },
            {"population", 120},
            {"depth", 30 },
            {"rowSpacing", 250}
        };

A helper function might make this a bit cleaner I guess but I'm not in favour of this approach.

The only other alternative is to do the superset of all arguments that have default (zero) values - also not really desirable.

@jbrider
Copy link
Contributor

jbrider commented Dec 14, 2021

If the sow method used an interface - by adding an additional function that leaves the current method in place to avoid breaking things, then you could extend it for different crops.
If the interface had most of the current defaults it would cause a minimum of issues and allow extension.

@hol353
Copy link
Contributor

hol353 commented Dec 15, 2021

@jbrider If I understand correctly, a generic sowing manager script (e.g. sow on fixed date) would then look like:

[Link]
IPlant plant;

if (plant.Name== "Wheat")
   plant.Sow(new SowData(cultivar: "hartog", population: 120, depth: 30, rowSpacing: 250));
else if (plant.Name == "Sorghum")
   plant.Sow(new SorghumData(cultivar: "buster", population: 120, depth: 30, rowSpacing: 250, ftn: 2));

Where SowData and SorghumData implement an interface.

I don't think we t want this. Old Apsim has this. We want a single sowing rule that works for all crops.

@jbrider
Copy link
Contributor

jbrider commented Dec 15, 2021

@hol353 good point - I was thinking about reading the values from it rather than creating it in a generic script.
Scripts that refer to ftn or skiprow are not necessarily going to be generic though.

For those values that are not included in the interface, a local variable could be used with a simple null check to setup a default if there is a generic script being used:
FTN = sowData as SorghumData?.FTN ?? 0.0;

Not pretty but might better than adding more properties to sowing data?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants