Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug where added cofactor reactions would get deleted #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonstrutz11
Copy link
Collaborator

When we remove reactions due to cofactor redundancy (Pickaxe._remove_cofactor_redunancy()), the following code can delete reactions created for previous cofactors in this for loop (for cpd_id in cofactors_as_cpds:).

Specficially, it can add them to this set on subsequent runs of the loop:

rxn_ids = set(
                self.compounds[cpd_id]["Product_of"]
                + self.compounds[cpd_id]["Reactant_in"]
            )
rxns_to_del = rxns_to_del.union(rxn_ids)

The code that creates new reactions is further down in _remove_cofactor_redunancy():

        # Reaction does not exist, generate new reaction
        else:
            cofactor_rxn = {
                "_id": cofactor_rxn_id,
                # give stoich and id of reactants/products
                "Reactants": [(s, r["_id"]) for s, r in reactants],
                "Products": [(s, p["_id"]) for s, p in products],
                "Operators": rxn["Operators"],
                "SMILES_rxn": rxn_text,
            }
        
            # Assign reaction to reactions dict
            self.reactions[cofactor_rxn_id] = cofactor_rxn

So even though this new reaction is created, sometimes it will then get added to the rxns_to_del set the next time the loop runs and will get subsequently deleted. I think this only happens if there are > 1 cofactor on one side of the reaction (so when the loop gets to the other cofactor it sees this reaction as one to delete), but I'm not 100% sure on this.

See commit changes for proposed fix. Basically I just make sure to not add any of the newly created cofactor reactions to the rxns_to_del set in subsequent runs of the loop.

This fix seems to increase final number of predicted reactions by around 3% and compounds by around 2% in my use case (and those 3% added reactions all make sense and I would expect them to be in the final set).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant