Skip to content

Variational data assimilation#122

Open
SCiarella wants to merge 4 commits into
mainfrom
Variational_DA
Open

Variational data assimilation#122
SCiarella wants to merge 4 commits into
mainfrom
Variational_DA

Conversation

@SCiarella
Copy link
Copy Markdown
Collaborator

@SCiarella SCiarella commented May 28, 2026

Closes #117 by adding a notebook dedicated to Variational data assimilation.

No new features have been added, so the changes are confined to the new notebook (+ docs)

@SCiarella SCiarella marked this pull request as ready for review May 28, 2026 11:56
@SCiarella SCiarella requested a review from SarahAlidoost May 28, 2026 11:56
@sonarqubecloud
Copy link
Copy Markdown

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 the notebook looks awesome 🥇 thanks! The explanations and plots are clear, I was able to run the notebook. There are some comments that should be fixed before the merge. Then we can ask others to have a look and provide feedback.

  1. Please add the diffwofost logo at the top, similar to other nbs
  2. in cell 1, in section "Variational data assimilation with diffWOFOST", where it says: "Earlier notebooks in this series demonstrated the Kalman-filter route ...", should be changed to "PCSE notebooks demonstrated the Kalman-filter route ...".
  3. In section "Background on data assimilation", the equation does not render correctly, please remove space after/before $$.
  4. In section "2.2.1 Defining the synthetic observations", the link to test data doesnot work because those test data are only available if someone runs pytest. Similar to other nbs, we should download them as:
import urllib.request

url = "https://raw.githubusercontent.com/ajwdewit/pcse/refs/heads/master/tests/test_data/test_waterlimitedproduction_wofost72_03.yaml"
filename = "test_waterlimitedproduction_wofost72_03.yaml"

urllib.request.urlretrieve(url, filename)
print(f"Downloaded: {filename}")

test_data_path = "test_waterlimitedproduction_wofost72_03.yaml"
  1. In section "2.2.1 Defining the synthetic observations", we don't need Truth_external_states as wofost72 does not need external variables.
  2. Cells number 39 and 43 are too long. Can you please break them into smaller cells where it is possible and add some inline comments.
  3. I like how you defined observations in "2.2.1 Defining the synthetic observations" 👍 This is something to be discussed and get feedback from research group.
  4. In cell 33, in class VariationalDiffWofost72, in run_model, where provider = copy.deepcopy(self.base_provider). We don't need this copy anymore right?

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.

[Task] Create a notebook to show variational DA with Wofost72_WLP_CWB

2 participants