Repository navigation
Microbial group inputs as a csv file rather than toml configuration - #1956
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 1928-not-providing-parameterisation-for-soil-microbial-groups-and-enzymes-doesnt-result-in-a-configuration-error #1956 +/- ##
====================================================================================================================================================
+ Coverage 88.91% 89.62% +0.70%
====================================================================================================================================================
Files 85 84 -1
Lines 9637 9534 -103
====================================================================================================================================================
- Hits 8569 8545 -24
+ Misses 1068 989 -79 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
David Orme (davidorme)
left a comment
David Orme (davidorme)
left a comment
There was a problem hiding this comment.
LGTM - I'm not a huge fan of the _enzyme_details attribute and have a suggestion there, but I don't know if those details need to be embedded beyond the clear use case in synthesis_nutrient_ratios.
| ````{dropdown} example_microbial_groups.csv | ||
| ```{literalinclude} ../../../../../virtual_ecosystem/example_data/data/example_microbial_groups.csv | ||
| ``` | ||
| ```` |
There was a problem hiding this comment.
I think we'd be better off using dynamically generated Markdown to avoid hardcoding these paths, but this isn't the only place we do this and it's not a big deal.
There was a problem hiding this comment.
yep both agree that the above is a bit of a mess and also that this isn't the place to fix it
| try: | ||
| microbes = [ | ||
| SoilMicrobe.model_validate(rec) for rec in data.to_dict(orient="records") | ||
| ] | ||
| for microbe in microbes: | ||
| microbe._enzyme_details = enzyme_classes | ||
| except ValidationError as excep: | ||
| raise ValueError(f"Invalid data in microbe parametrisation CSV file: {excep!s}") | ||
|
|
||
| # Check the set of microbes for duplicates | ||
| provided_microbes = [e.name for e in microbes] | ||
| set_provided_microbes = set(provided_microbes) | ||
|
|
||
| if len(set_provided_microbes) != len(provided_microbes): | ||
| raise ValueError( | ||
| "Duplicated microbial groups in microbial parametrisation data" | ||
| ) | ||
|
|
||
| if not set_provided_microbes == set(get_args(REQUIRED_MICROBIAL_GROUPS)): | ||
| raise ValueError( | ||
| f"The microbial groups provided in the microbial groups definition file " | ||
| f"({set_provided_microbes!r}) do not match the required set of microbial " | ||
| f"groups ({REQUIRED_MICROBIAL_GROUPS!r})" | ||
| ) | ||
|
|
||
| # Package into a dict and return | ||
| return {r.name: r for r in microbes} |
There was a problem hiding this comment.
I don't know if _enzyme_details is accessed elsewhere, but at the moment it looks like it is stuck in purely to support a call to synthesis_nutrient_ratios.
I'd drop it as a private attribute and make enzyme_details an argument in the call to synthesis_nutrient_ratios and then have an explicit call to synthesis_nutrient_ratios(enzyme_details) here in load_microbes. That does mean you can't calculate those ratios later, but there is then an explicit point in the code that populates those values and you avoid the extra property.
There was a problem hiding this comment.
Wouldn't that make synthesis_nutrient_ratios a method rather than an attribute/property though? So downstream code would have to go from e.g.
nitrogen_limit = (
functional_group.synthesis_nutrient_ratios["nitrogen"] * nitrogen_gain_max
)
to
synthesis_ratios = functional_group.synthesis_nutrient_ratios(enzyme_details)
nitrogen_limit = (
synthesis_ratios["nitrogen"] * nitrogen_gain_max
)
with the enzyme details having to be passed to wherever the attribute needs to be used
There was a problem hiding this comment.
Huh. Yeah. That's right. OK - if enzyme details are only used at this point, I still think it's odd to attach them. I think there are two options (well, three, if you're happy to live with my unease 😁 ):
-
Make them an attribute that defaults to None and then have a method to populate:
-
... but that leaves some odd holes - you have to run
populate_band you can in theory populatebat__init__within any old junk and then notpopulate_b. The pydantic implementation of dataclasses may be cleaner here.
In [2]: from dataclasses import InitVar
...: from pydantic import BaseModel, Field
...: from pydantic.dataclasses import dataclass
...:
...: class Test(BaseModel):
...:
...: a: int
...: b: int | None = None
...:
...: def populate_b(self, x: int) -> None:
...:
...: self.b = self.a * x
...:
...: t = Test(a=1)
...: t.populate_b(x=3)
...: t
...:
Out[2]: Test(a=1, b=3)
In [3]:
...: @dataclass
...: class Test2():
...:
...: a: int
...: b: int = Field(init=False)
...: x: InitVar[int]
...:
...: def __post_init__(self, x: int) -> None:
...:
...: self.b = self.a * x
...:
...: t2 = Test2(a=1, x=3)
...: t2
Out[3]: Test2(a=1, b=3)
In [4]: Note that this is still a pydantic model so you get validation on the inputs.
In [4]: t2 = Test2(a=1.5, x=3)
---------------------------------------------------------------------------
ValidationError Traceback (most recent call last)
Cell In[4], line 1
----> 1 t2 = Test2(a=1.5, x=3)
File ~/Library/Caches/pypoetry/virtualenvs/virtual-ecosystem-In6MogPy-py3.13/lib/python3.13/site-packages/pydantic/_internal/_dataclasses.py:121, in complete_dataclass.<locals>.__init__(__dataclass_self__, *args, **kwargs)
119 __tracebackhide__ = True
120 s = __dataclass_self__
--> 121 s.__pydantic_validator__.validate_python(ArgsKwargs(args, kwargs), self_instance=s)
ValidationError: 1 validation error for Test2
a
Input should be a valid integer, got a number with a fractional part [type=int_from_float, input_value=1.5, input_type=float]
For further information visit https://errors.pydantic.dev/2.14/v/int_from_float
9c366e3
into
1928-not-providing-parameterisation-for-soil-microbial-groups-and-enzymes-doesnt-result-in-a-configuration-error
Description
This PR extends the work done in #1947, so that soil microbial groups are also read in from a csv file.
To make the changes clearer I'm setting this PR to merge to the branch used into #1947. My intention is to merge both of them to the feature branch, and then do a final PR against that.
Fixes #1542
Fixes #1954
Type of change
Key checklist
pre-commitchecks:$ pre-commit run -a$ poetry run pytestFurther checks
data science team included as reviewers?