Conversation
augeorge
commented
Feb 7, 2024
augeorge
commented
- new data class for Phenotype Observations
- new functions for:
- create cobra model from stoich matrix
- get dual and primal constraints (incomplete)
- get decision variables
- new class for CROP problem (incomplete)
- example notebook and model for tests (incomplete)
- unit tests and fixtures covering new functions (incomplete)
djinnome
left a comment
djinnome
left a comment
There was a problem hiding this comment.
This is definitely looking better, but I think we should make smaller PRs so that main only has working tests that always pass. Maybe split this PR into smaller pieces?
| return cb.io.load_json_model("./tests/ABC_toy_model_3.json") | ||
|
|
||
| @pytest.fixture | ||
| def actual_model(): |
There was a problem hiding this comment.
Fixtures should be expected, not actual.
| return flux_dual | ||
|
|
||
| @pytest.fixture | ||
| def flux(): |
There was a problem hiding this comment.
Should this be expected_flux?
tests/fixtures.py
Outdated
| return flux | ||
|
|
||
|
|
||
| def metabolite_equal(expected:cb.core.Metabolite, |
There was a problem hiding this comment.
Why is there a unit test in fixtures?
There was a problem hiding this comment.
Oh, I see: it is a helper function for testing equality. I think this should be called assert_metabolite_equal and assert_reaction_equal and assert_constraint_equal
There was a problem hiding this comment.
Oh never mind, I like how you do assert metabolite_equal(expected, actual)
tests/test_crop.py
Outdated
| obj_func = model.slim_optimize() | ||
| expected_constraint = None # fix | ||
| # compare with expected model expressions | ||
| assert actual_constraint == expected_constraint |
There was a problem hiding this comment.
should this be assert contraint_equal(actual_constrainte, expected_constraint) or does this actually work?
| flux_dual | ||
| ): | ||
| """Test getting the dual flux constraints (i.e., LB <= r_nogrowth <= UB)""" | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Should you go ahead and implement this?
| flux | ||
| ): | ||
| """Test getting the steady state flux constraints (i.e., Sv=0) """ | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Should you go ahead and implement this?
There was a problem hiding this comment.
Should we really merge this into main?
… eqnarray with align
2 participants
