Skip to content

Make phenology differntiable and vectorized#57

Merged
SarahAlidoost merged 21 commits into
mainfrom
phenology_diff
Dec 9, 2025
Merged

Make phenology differntiable and vectorized#57
SarahAlidoost merged 21 commits into
mainfrom
phenology_diff

Conversation

@SCiarella
Copy link
Copy Markdown
Collaborator

@SCiarella SCiarella commented Nov 24, 2025

Targets issue #40.

Notes:

  • the variables VERNFAC and VERNR are not part of the 'State variables' so I have excluded them from the output consistency assertion, since the engine does not store them
  • not yet vectorized over CROP_START_TYPE and CROP_END_TYPE
  • _send_signal needs updates now that we could be processing a vector of crops in parallel
  • I am currently using a trick to pass the test of the phenology model, by considering the stopping condition only for the last input element (if it is a tensor).
  • IDSL is a switch, so I have not made it differentiable
  • DVSI is an initial parameter, so I have not made it differentiable

To do:

  • VERNSAT, VERNBASE and VERNDVS are not yet differentiable (tests fail)
  • wofost integration tests (patch) fail

@SCiarella SCiarella marked this pull request as draft November 25, 2025 12:03
@SarahAlidoost
Copy link
Copy Markdown
Collaborator

SarahAlidoost commented Nov 26, 2025

Targets issue #40.

Notes:

  • the variables VERNFAC and VERNR are not part of the 'State variables' so I have excluded them from the output consistency assertion, since the engine does not store them

@SCiarella that's correct. I found that there is a phenology config file also in pcse, see here. The OUTPUT_VARS are listed there. Could we add "TSUM","TSUME" to our phenology config as well?

Comment thread tests/physical_models/crop/test_phenology.py Outdated
Comment thread docs/api_reference.md Outdated
SCiarella and others added 2 commits November 26, 2025 11:37
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Comment thread docs/api_reference.md Outdated
@SarahAlidoost
Copy link
Copy Markdown
Collaborator

SarahAlidoost commented Dec 1, 2025

Targets issue #40.

Notes:

  • the variables VERNFAC and VERNR are not part of the 'State variables' so I have excluded them from the output consistency assertion, since the engine does not store them

This is in line with pcse config file 👍

  • not yet vectorized over CROP_START_TYPE and CROP_END_TYPE

We can define them as numpy arrays of string and fix the if conditions. right?
see #60

  • _send_signal needs updates now that we could be processing a vector of crops in parallel

can you please explain this in a separate issue that we dont forget it?
see #60

  • I am currently using a trick to pass the test of the phenology model, by considering the stopping condition only for the last input element (if it is a tensor).

Need discussion.

  • IDSL is a switch, so I have not made it differentiable
  • DVSI is an initial parameter, so I have not made it differentiable

nice! 👍 We can add this to the documentation.

To do:

  • VERNSAT, VERNBASE and VERNDVS are not yet differentiable (tests fail)

Tha's okay. As we discussed, the parameters of DVS_Phenology will be differentiable

  • wofost integration tests (patch) fail

this is now fixed. see my commit.

@SarahAlidoost SarahAlidoost marked this pull request as ready for review December 5, 2025 11:23
Copy link
Copy Markdown
Collaborator

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@SCiarella thanks for the nice work! 👍 please see my comments and let's have a chat about some of the changes I made 😄

Comment thread src/diffwofost/physical_models/crop/phenology.py
Comment thread src/diffwofost/physical_models/crop/phenology.py Outdated
Comment thread tests/physical_models/crop/test_phenology.py Outdated
SCiarella and others added 3 commits December 8, 2025 12:43
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Dec 8, 2025

Copy link
Copy Markdown
Collaborator

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@SCiarella thanks for addressing the comments 👍

@SarahAlidoost SarahAlidoost merged commit 14a993c into main Dec 9, 2025
11 checks passed
@SarahAlidoost SarahAlidoost deleted the phenology_diff branch December 9, 2025 10:29
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