-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: update GPRs for Cytochrome C Oxidoreductase and remove MAR04456 that is duplicated to MAR04458+MAR04589 #532
Conversation
@Devlin-Moyer @feiranl maybe check again this PR after more changes were made |
- avoid ROX version complex IV from carrying flux - remove COX8C (ENSG00000187581)
model/Human-GEM.yml
Outdated
@@ -302273,7 +302273,7 @@ | |||
- MAM02630m: -1 | |||
- MAM02631m: 0.02 | |||
- lower_bound: 0 | |||
- upper_bound: 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it alright to leave the bound as it is before? I'm not certain which reaction will carry the flux, whether this ROS one or the regular one. I personally vote for keeping them both open. In the step of specific model generation, the user can remove either one of these options. Blocking them in the generic model could lead to unnecessary issues in the specific model simulation, making it difficult to identify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, only one version should be left unblocked by default. Until this thread, it didn't even occur to me that Complex IV effectively has twice as much flux capacity as the rest of the ETC reactions by default, since the other ETC reactions don't (as far as I know) have comparable ROS-producing duplicate versions. Nothing about MAR06914 suggests that it has an ROS-producing duplicate in MAR13081, and I think it'd it'd be much harder to figure out why it seems like there's more flux than you bargained for going through Complex IV (e.g. you only changed the bound on MAR06914 and did not know MAR13081 existed) than to figure out why Complex IV isn't producing ROS (which you can figure out easily by just looking at the reaction string for MAR06914 and seeing no ROS production)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @Devlin-Moyer, this reaction can easily be missed, and may cause confusion if one is not aware of the two pathways. I can't think of a scenario where deactivating it would cause any issues, but I can think of a few annoyances that could be caused by keeping both reactions open. So I would lean toward deactivating it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, at the moment, nobody seems to know where the stoichiometric coefficients in MAR13081 came from, but the stoichiometric coefficients in MAR06914 clearly come from the associated reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, in that case, could we change the name or the subsystem to indicate the reason why we block this at least point to ROS or similar commet. Otherwise, in one or two years, we forget why we block this rxn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@feiranl the reason the model is maintained on GitHub is precisely so that in 2 years (or more) we can search this repository to find out why changes were made. I would not recommend changing the subsystem just for tracking reasons, but if there is another subsystem which is actually deemed more appropriate, then of course that change can be considered. It's also fine to update the name of the reaction to be more descriptive, if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the comment above, all others looks good to me. Well done!
Main improvements in this PR:
This PR resolves two issues (this should be avoided):
I hereby confirm that I have:
develop
as a target branch