Skip to content

Use all test data#54

Merged
SarahAlidoost merged 10 commits into
mainfrom
use_all_test_data
Nov 17, 2025
Merged

Use all test data#54
SarahAlidoost merged 10 commits into
mainfrom
use_all_test_data

Conversation

@SarahAlidoost
Copy link
Copy Markdown
Collaborator

@SarahAlidoost SarahAlidoost commented Nov 2, 2025

closes #27

This PR:

  • reads test data directly from its github repo, no need to download them
  • uses all data in tests for checking the correctness of the model
  • fixes a few bugs in leaf dynamics

@SarahAlidoost SarahAlidoost marked this pull request as ready for review November 3, 2025 12:59
Copy link
Copy Markdown
Collaborator

@fnattino fnattino left a comment

Choose a reason for hiding this comment

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

Hi @SarahAlidoost nice and clean! I have left one main comment related to some changes in leaf dynamics, I'd be interested to know what you think about that.

The current approach for testing is very nice, now we don't have to host the test data anymore as part of this repository. The drawback is that each test that requires external data fetches data from remote (actually the same dataset can also be fetched multiple times in a single test run). This will make tests slower (probably not a big deal) but also slightly less reliable (connections hiccups, timeouts).

We can simply try out how things work and reassess if we see that tests randomly fail because of remote access limitations. If this happen, an alternative approach could be to clone the PCSE repository before running tests and to set the path to the test data dir to be used in tests via some environment variable or config file. Much less elegant, but it would make sure data is fetched only once, and, if you run tests locally, you don't have to continuously download data from a remote.

Comment thread src/diffwofost/physical_models/crop/leaf_dynamics.py Outdated
Comment thread src/diffwofost/physical_models/crop/leaf_dynamics.py
Comment thread src/diffwofost/physical_models/crop/leaf_dynamics.py
Comment thread src/diffwofost/physical_models/crop/leaf_dynamics.py
@SarahAlidoost
Copy link
Copy Markdown
Collaborator Author

Hi @SarahAlidoost nice and clean! I have left one main comment related to some changes in leaf dynamics, I'd be interested to know what you think about that.

The current approach for testing is very nice, now we don't have to host the test data anymore as part of this repository. The drawback is that each test that requires external data fetches data from remote (actually the same dataset can also be fetched multiple times in a single test run). This will make tests slower (probably not a big deal) but also slightly less reliable (connections hiccups, timeouts).

We can simply try out how things work and reassess if we see that tests randomly fail because of remote access limitations. If this happen, an alternative approach could be to clone the PCSE repository before running tests and to set the path to the test data dir to be used in tests via some environment variable or config file. Much less elegant, but it would make sure data is fetched only once, and, if you run tests locally, you don't have to continuously download data from a remote.

you are right! thanks for pointing this out. A better approach is to ask pytest to download the test data for the test session. I added a conftest.py to do this, see my last commit. Since the test_data folder in pcse isnot a zip file, I had to specify the filenames. I think for now it is okay. Later, we can ask them to provide a zip file too.

@SarahAlidoost
Copy link
Copy Markdown
Collaborator Author

Hi @SarahAlidoost nice and clean! I have left one main comment related to some changes in leaf dynamics, I'd be interested to know what you think about that.

@fnattino thanks a lot for your comments and suggestions 🥇, I addressed them. Please let me know if something else should be fixed :bowtie:

Copy link
Copy Markdown
Collaborator

@fnattino fnattino left a comment

Choose a reason for hiding this comment

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

All looks good, thanks for the clarifications!

Comment thread tests/physical_models/conftest.py
@sonarqubecloud
Copy link
Copy Markdown

@SarahAlidoost SarahAlidoost merged commit b32ac13 into main Nov 17, 2025
11 checks passed
@SarahAlidoost SarahAlidoost deleted the use_all_test_data branch November 17, 2025 10:02
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]: use all test data for unit tests

2 participants