CWorthy-ocean / C-Star

C-Star is a python package for setting up and running ocean model simulations, with a particular focus on marine carbon dioxide removal (mCDR) applications.
https://c-star.readthedocs.io
10 stars 4 forks source link

Update test case #108

Closed dafyddstephenson closed 2 weeks ago

dafyddstephenson commented 3 weeks ago

This PR aims to update the current pytest framework with a new test case that is defined on a smaller grid and integrates with roms-tools with minimal requirements for external data.

It was initially opened as a draft, the progression can be followed below.

Closes #106 Closes #104 Closes #99

dafyddstephenson commented 3 weeks ago

@TomNicholas I had a crack at adding local sources back in today (as they can now be made available cheaply) but the two different test blueprints have different locations for input_datasets, so hardcoding the location as is currently implemented isn't ideal.

In general though I think it'd be good to refactor this to make it a bit less complex, as right now the call sequence out of the primary test is blueprint_as_path -> blueprint_as_dict -> set_locations -> modify_yaml -> condition_func -> modify_func (I think) where condition_func and modify_func are set dynamically and can take multiple forms. This is difficult to follow and makes tracking changes through this sequence very complicated.

dafyddstephenson commented 3 weeks ago

OK, I think this is ready to go. Sorry to basically start over @TomNicholas (what was there helped a lot with wrapping my head around what to do), I think modifying a temporary copy of the yaml before making a Case from it reduces the complexity a lot, so blueprints/fixtures.py is now down to a single short function. There are two more fixtures in roms/fixtures.py to fetch external data and a tests/config.py for defining globals, including the dict TEST_CONFIG over whose keys the primary test is parameterised.

I managed to sort out the pytest ignore issue, so it should work fine from anywhere now. Happy to set up a call to walk through if anything needs explaining. Looking forward to iterating and getting this in :)

NoraLoose commented 3 weeks ago

As ROMS-Tools develops the NetCDF data (maybe even the NetCDF file names) produced by YAML files like https://github.com/CWorthy-ocean/cstar_blueprint_test_case/blob/main/roms_tools_yaml_files/roms_bry_bgc.yaml will change as well.

Should we read the header https://github.com/CWorthy-ocean/cstar_blueprint_test_case/blob/d7996aea8d4fd4ca4148b34d2d898f019c90a8ff/roms_tools_yaml_files/roms_bry_bgc.yaml#L1-L3 and then check out the appropriate version of ROMS-Tools to get the expected behavior?

dafyddstephenson commented 3 weeks ago

As in, make the roms-tools version associated with the test case the version that we fix as the dependency?

NoraLoose commented 3 weeks ago

As in, make the roms-tools version associated with the test case the version that we fix as the dependency?

This could be an option. Or we just fix the roms-tools version when running the tests? I'm adding new features to ROMS-Tools continuously, and wouldn't want the C-Star user to miss out on them. But on the other hand, we probably want to test the ROMS-Tools integration with the same version that the user will be using. Not sure...

dafyddstephenson commented 3 weeks ago

hadn't occurred to me that we could have a separate version for the tests, but yes, that makes a lot more sense.

NoraLoose commented 3 weeks ago

Maybe we could have a GitHub workflow that updates the data in https://github.com/CWorthy-ocean/cstar_blueprint_test_case? Too complicated?

dafyddstephenson commented 3 weeks ago

I think it makes more sense to keep it (relatively) static given that it's supposedly another "blueprint". We can issue updates but probably not have it continuously change

NoraLoose commented 3 weeks ago

Or should it be a feature of C-Star (beta version) to switch to the ROMS-Tools version given in the header? To ensure reproducibility of the ROMS input data for a given blueprint with ROMS-Tools YAML files?

dafyddstephenson commented 3 weeks ago

Is there a way to hot-switch the version of a dependency without reinstalling/rebuilding your environment...?

NoraLoose commented 3 weeks ago

I think we need to overwrite via

pip install roms-tools==x.x.x
dafyddstephenson commented 3 weeks ago

Not sure I understand how that would be implemented, perhaps we can discuss on a dedicated issue?

TomNicholas commented 3 weeks ago

Or should it be a feature of C-Star (beta version) to switch to the ROMS-Tools version given in the header?

I'm not sure I fully follow this conversation, but one library changing the version of another library sounds not good. Shouldn't we just be aiming to get to a point where changes in roms-tools API don't affect the reproducibility of the result?

NoraLoose commented 3 weeks ago

Shouldn't we just be aiming to get to a point where changes in roms-tools API don't affect the reproducibility of the result?

ROMS-Tools is not at that point yet. For example, we just exchanged the filling method (https://github.com/CWorthy-ocean/roms-tools/pull/140), which altered the results. Next on the list are:

all of which will again alter the results.

Btw, the continuous alteration of results also causes a problem within ROMS-Tools itself: https://github.com/CWorthy-ocean/roms-tools/issues/152

TomNicholas commented 3 weeks ago

Great progress @dafyddstephenson ! I think the fixture structure could be re-jigged a little still.

dafyddstephenson commented 3 weeks ago

@NoraLoose Have added a tests optional dependency list to the pyproject.toml and fixed roms_tools at 1.4.0 there. @TomNicholas added a docstring to the main test and restricted the responsibility of the fixture to just modifying the blueprint template (the case is now created within the test itself)