Remove deprecated manipulation2 #1208
Conversation
removed find_gene_knockout_reactions from delete.py removed _find_gene_knockout_reactions_fast, _gene_knockout_computation, _get_removed from test_delete.py
There was a problem hiding this comment.
Some minor suggestions, otherwise this looks good. I think the knock_out_model_genes function makes sense as it conveniently returns the list of affected reactions. I'm not super happy with the name but it's descriptive and I don't have a better suggestion. Good work 👍🏼
src/cobra/manipulation/delete.py
Outdated
| new_bounds = model.reactions.list_attr("bounds") | ||
| reaction_list = list() | ||
| for i, rxn in enumerate(model.reactions): | ||
| if not ( | ||
| np.isclose(orig_bounds[i][0], new_bounds[i][0]) | ||
| and np.isclose(orig_bounds[i][1], new_bounds[i][1]) | ||
| ): | ||
| reaction_list.append(rxn) | ||
| return reaction_list |
There was a problem hiding this comment.
Even though it increases the computational work, I think a version more in line with the object-oriented design is:
| new_bounds = model.reactions.list_attr("bounds") | |
| reaction_list = list() | |
| for i, rxn in enumerate(model.reactions): | |
| if not ( | |
| np.isclose(orig_bounds[i][0], new_bounds[i][0]) | |
| and np.isclose(orig_bounds[i][1], new_bounds[i][1]) | |
| ): | |
| reaction_list.append(rxn) | |
| return reaction_list | |
| return [rxn for rxn in itertools.chain.from_iterable((gene.reactions for gene in gene_list)) if not rxn.functional] |
There was a problem hiding this comment.
Why are there two parenthesis?
There was a problem hiding this comment.
Also, why set? In the get_by_any above?
There was a problem hiding this comment.
Not sure gene.reactions as written would work, since it can accept strings
There was a problem hiding this comment.
Double parenthesis was outer for the function call and the inner for a generator statement. This was assuming that gene_list would be the output of get_by_any, sorry, my comment wasn't clear.
I used a set to avoid duplicates which are in principle possible with a list and get_by_any.
There was a problem hiding this comment.
for gene in model.genes.get_by_any(set(gene_list)): fails due to type error
return the_id in self._dict
E TypeError: unhashable type: 'set'
../manipulation/delete.py:86: in knock_out_model_genes
for gene in model.genes.get_by_any(set(gene_list)):
../core/dictlist.py:108: in get_by_any
return [get_item(item) for item in iterable]
../core/dictlist.py:108: in
return [get_item(item) for item in iterable]
../core/dictlist.py:101: in get_item
../core/dictlist.py:379: TypeError
There was a problem hiding this comment.
I think the current version [https://github.com//pull/1208/commits/ae1fdbe5beea58cc453253111914a1bd27e7b750] is the best, and object-oriented. Fixed other comments.
Co-authored-by: Moritz E. Beber <[email protected]>
Codecov Report
@@ Coverage Diff @@
## devel #1208 +/- ##
==========================================
- Coverage 84.23% 83.62% -0.61%
==========================================
Files 65 65
Lines 5549 5497 -52
Branches 1287 1263 -24
==========================================
- Hits 4674 4597 -77
- Misses 564 598 +34
+ Partials 311 302 -9
Continue to review full report at Codecov.
|
src/cobra/manipulation/delete.py
Outdated
| new_bounds = model.reactions.list_attr("bounds") | ||
| reaction_list = list() | ||
| for i, rxn in enumerate(model.reactions): | ||
| if not ( | ||
| np.isclose(orig_bounds[i][0], new_bounds[i][0]) | ||
| and np.isclose(orig_bounds[i][1], new_bounds[i][1]) | ||
| ): | ||
| reaction_list.append(rxn) | ||
| return reaction_list |
There was a problem hiding this comment.
Why are there two parenthesis?
Added tests to make sure that int and cobra.Gene will work as well
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
Added knock_out_model_genes()
Modified test_delete_genes to perform the same tests with the new functions.
This is a cleaner version of #1200
One decision that needs to be made is about `knock_out_model_genes() which is a simple helper function for people who want to knock out lots of genes, and aren't good with writing the list comprehension on their own.