Skip to content

Run multiple crops - first work on engine#124

Merged
fnattino merged 13 commits into
mainfrom
60-different-crops
Jun 15, 2026
Merged

Run multiple crops - first work on engine#124
fnattino merged 13 commits into
mainfrom
60-different-crops

Conversation

@fnattino

@fnattino fnattino commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Addressing first few points of the work on engine in #60 (comment), in particular:

  • we need to keep the crop instance alive for the full length of the simulation. This will make the crop modelling more consistent to the soil modelling (the soil instance also lives for the full extent of the simulation), but it will break the way in which crop rotation is currently implemented.

The crop instance is now created when the engine is setup and kept alive for the full length of the simulation.

  • we should simplify the interface of the parameter provider, maybe allowing a plain dictionary to be used as input? This way we could create parameter providers for arbitrary sets of crops.

One can now use a plain dictionary as parameter provider. Note that a dict-like access is now expected for the parameter provider, which is currently not supported by PCSE. A PR should fix this (ajwdewit/pcse#121), until that moment I have added a temporary implementation here.

  • we should make sure that simulations can run with an "empty" agromanagement, provided that the necessary parameters are provided as input (i.e. via the parameter provider).

We can actually already run an almost empty agromanagement, only start date and end date are necessarily extracted from it. So an agromanagement like the following works for now: [{datetime.date(2010, 4, 16): None}, {datetime.date(2010, 12, 31): None}]. We should probably improve the interface a bit, and maybe we could consider making the agromanagement optional if start date and end date are provided as separate input arguments. Also this, should be improved as a separate PR.

What is definitely missing here and should be implemented as a separate PR is the tensor-based weather data provider.

@fnattino fnattino changed the title Run multiple crops Run multiple crops - first work on engine Jun 10, 2026
@fnattino fnattino marked this pull request as ready for review June 10, 2026 12:18
@fnattino fnattino requested a review from SarahAlidoost June 10, 2026 12:19
Comment thread src/diffwofost/physical_models/parameter_providers.py
Comment thread tests/physical_models/test_engine.py

@SarahAlidoost SarahAlidoost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fnattino thanks! 👍 it looks good. I left two comments. The main one is to test the multiple crops with the test data. Since models can support vectors, we can pass two different crops to one model. I suggest using phenology model. If this needs too much changes, it can be implemented in a different PR.

@fnattino

fnattino commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for having a look @SarahAlidoost! Concerning the new test on multiple crops, I think it would be easier to implement it when other pieces of the engine will be there (e.g. the weather provider with support for tensors), because the two crops will need different weather data as well (working on #131)

@sonarqubecloud

Copy link
Copy Markdown

@SarahAlidoost

Copy link
Copy Markdown
Collaborator

Thanks for having a look @SarahAlidoost! Concerning the new test on multiple crops, I think it would be easier to implement it when other pieces of the engine will be there (e.g. the weather provider with support for tensors), because the two crops will need different weather data as well (working on #131)

Thanks. Then feel free to merge this one.

@fnattino fnattino merged commit 89a3335 into main Jun 15, 2026
11 checks passed
@fnattino fnattino deleted the 60-different-crops branch June 15, 2026 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants