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

doc-func: change some variables in function MissingTransDeadEnd to clarify #195

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

feiranl
Copy link
Collaborator

@feiranl feiranl commented Jul 22, 2019

Main improvements in this PR:

change some variable names and clarify meaning of each variable

I hereby confirm that I have:

  • Tested my code with all requirements for running the model
  • Selected devel as a target branch (top left drop-down menu)
  • If needed, asked first in the Gitter chat room about this PR

@edkerk edkerk changed the title doc-func: change some varibales in function MissingTransDeadEnd to clarify doc-func: change some variables in function MissingTransDeadEnd to clarify Jul 23, 2019
@BenjaSanchez BenjaSanchez self-assigned this Jul 24, 2019
Copy link
Contributor

@BenjaSanchez BenjaSanchez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR Feiran, it does help to understand better. A couple lingering questions/observations are:

  1. Why is DEM_reason bigger than DEM? Should't they be of the same size?
  2. Note that even though you are trying to save at the end DEM_maysolve and DEM_nosolve, as you are not returning them as output they are not stored when the function is called.

@feiranl
Copy link
Collaborator Author

feiranl commented Jul 24, 2019

  1. Becasue there may be more than one way to fix the gap.
    For example: there are one met appears in three compartmets: A[c] A[m] A[er]
    A[c] and A[m] are not deadend mets, A[er] is the deadend met. There are two ways of dismiss the deadend of A[er] by adding either one transport rxns from A[c] and A[m]. When you unique the first column of DEM_reason, you will get the same number with DEM.

  2. Yes, Only the canbesolved will be evaluated manually at the end. Therefore, in order not to return so many variables causing confusion, so I combined DEM_maysolve and DEM_nosolve toghther to be DEM_reason. If people are interested, they can check this variable to see the reason/possible solution. I don't delete the variable at the end, because sometime I need to check the consistency of canbesolved with DEM_maysolve.

@BenjaSanchez
Copy link
Contributor

  1. Thanks for clarifying it here :)
  2. What I mean is that this line is redundant, because as MissingTransDeadEnd is a function, the only variables that will be returned are model, canbesolved, DEM and DEM_reason. Everything else will be cleared by default.

@BenjaSanchez BenjaSanchez merged commit eb07620 into devel Jul 26, 2019
@BenjaSanchez BenjaSanchez mentioned this pull request Jul 26, 2019
@BenjaSanchez BenjaSanchez deleted the fix_gapfilling_confusion branch October 8, 2019 14:33
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.

2 participants