Remove depreceated manipulation #1200
Conversation
removed find_gene_knockout_reactions from delete.py removed _find_gene_knockout_reactions_fast, _gene_knockout_computation, _get_removed from test_delete.py
Codecov Report
@@ Coverage Diff @@
## devel #1200 +/- ##
==========================================
- Coverage 84.03% 83.10% -0.94%
==========================================
Files 65 65
Lines 5375 5327 -48
Branches 1241 1182 -59
==========================================
- Hits 4517 4427 -90
- Misses 551 598 +47
+ Partials 307 302 -5
Continue to review full report at Codecov.
|
|
@Midnighter @synchon @cdiener - can I get a review of this from one of you please? |
| new_bounds = model.reactions.list_attr("bounds") | ||
| reaction_list = list() | ||
| for i, rxn in enumerate(model.reactions): | ||
| if orig_bounds[i] != new_bounds[i]: |
There was a problem hiding this comment.
Exact comparison with floating point numbers is never a good idea.
There was a problem hiding this comment.
So math.isclose(), maybe with the solver defined tolerance?
Something else?
https://docs.python.org/3/whatsnew/3.5.html#pep-485-a-function-for-testing-approximate-equality
There was a problem hiding this comment.
Since numpy is a direct dependency you can also use numpy.isclose if you like. And I don't think the solver tolerance has an impact on bound equivalence unless I overlooked something.
|
|
||
|
|
||
| def find_gene_knockout_reactions( | ||
| def knock_out_model_genes( |
There was a problem hiding this comment.
This looks like a major API change and not what the PR title implies. Can you add some more comment to the PR description why that was necessary?
There was a problem hiding this comment.
Sure.
My reasoning was - delete_model_genes() is marked as deprecated, and should be removed. However, it might be possible people want to set all reactions of a gene to zero.
I couldn't see any other function doing it, so I added a new function that will do so in a context specific manner.
If there is one that does it since remove_model_genes removes genes from model, I missed it and would be happy to point to it.
There was a problem hiding this comment.
All reactions of a gene are already knocked out with a normal Gene.knockout. Did you mean knockout several genes in a model? The question there would if we really need a helper since [g.knockout() for g in gene_list()] is already pretty concise and will work with the context manager.
There was a problem hiding this comment.
You're right. Several genes in a model.
My function is simply a helper which returns a list, and so is functionally equivalent to delete_model_genes() only in a context aware manner and with updated code.
Do we want to have this helper for people who aren't that good with writing python code? If not , I can just delete it.
There was a problem hiding this comment.
@cdiener - any decision on this?
Keep or remove?
| ) -> List["Reaction"]: | ||
| """Identify reactions which will be disabled when genes are knocked out. | ||
| """Temporarily remove the effect of genes in `gene_list`. |
There was a problem hiding this comment.
Maybe better to say "Disable the genes in gene_list." It's a bit unclear what "temporarily removing the effect" of a gene would mean.
There was a problem hiding this comment.
Will fix once other comments are answered, as to not spam the github actors
There was a problem hiding this comment.
Again. The problem of updates. I can rebase it on the new devel so there won't be a million commits and re-commits.
|
I was hoping we could fix the CI deadlocks before that, sorry for the delay. |
…function is different. Also, the documentation states that "warnings.warn() in library code if the issue is avoidable and the client application should be modified to eliminate the warning" And malformed GPR should be modified in the client application (by modifying the input).
|
see #1208 |
Removed deprecated manipulation functions including undelete_model_genes, get_compiled_gene_reaction_rules, find_gene_knockout_reactions
Removed related functions from test_delete.py
Removed trimmed field from model
Modified test_delete_genes to perform the same tests with the new functions.