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

632 import concentration based sbml files #633

Merged
merged 10 commits into from
Oct 7, 2021

Conversation

abdelr
Copy link
Member

@abdelr abdelr commented Oct 7, 2021

Fixes #632 (Import from SBML File with concentration based reactions)

@abdelr abdelr requested a review from msevestre October 7, 2021 02:43
@abdelr abdelr self-assigned this Oct 7, 2021
@abdelr abdelr marked this pull request as draft October 7, 2021 02:43
molInfo.SetDimension(newDim);
msv.Formula = _context.Create<ExplicitFormula>($"{msv.Name}_0").WithName($"{msv.Name}_0").WithDimension(amountDimension).WithFormulaString($"{baseValue.value} * {Constants.VOLUME_ALIAS}");
msv.Formula.AddObjectPath(
ObjectPathFactory.CreateFormulaUsablePathFrom(ObjectPath.PARENT_CONTAINER, Constants.Parameters.VOLUME)
Copy link
Member Author

Choose a reason for hiding this comment

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

@msevestre Check the change in the call. This is why the Volume was not found before. It should not be one single string but the list of all individual paths

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, it would be very nice to use the code base for concentration based reactions, but the way the importer is written it is not trivial to me. I would propose to keep it in mind for the future but just use this solution for now.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good. What about the reaction rate? Aren;t they expecting concentration?

Copy link
Member Author

@abdelr abdelr Oct 7, 2021

Choose a reason for hiding this comment

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

@msevestre I think this is related to the second PR, right?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know lol.

@abdelr abdelr marked this pull request as ready for review October 7, 2021 03:57
@@ -191,6 +196,7 @@ private void SetMoleculeStartValues(Model model)
if (molInfo.GetContainer().Any(x => x.Name == msv.ContainerPath.LastOrDefault()))
{
msv.IsPresent = true;
msv.NegativeValuesAllowed = true;
Copy link
Member

Choose a reason for hiding this comment

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

for 99.9% of MSV the reasonable setting is NegativeValuesAllowed = false
If the msv entry is one of the remaining 0.1% or if there are some numerical issues - it still can be set to true by user.
@PavelBal

Copy link
Member Author

@abdelr abdelr Oct 7, 2021

Choose a reason for hiding this comment

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

@Yuri05 is right, it can be manually set.

@abdelr abdelr requested a review from Yuri05 October 7, 2021 10:26
molInfo.SetDimension(newDim);
msv.Formula = _context.Create<ExplicitFormula>($"{msv.Name}_0").WithName($"{msv.Name}_0").WithDimension(amountDimension).WithFormulaString($"{baseValue.value} * {Constants.VOLUME_ALIAS}");
msv.Formula.AddObjectPath(
ObjectPathFactory.CreateFormulaUsablePathFrom(ObjectPath.PARENT_CONTAINER, Constants.Parameters.VOLUME)
Copy link
Member

Choose a reason for hiding this comment

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

sounds good. What about the reaction rate? Aren;t they expecting concentration?

{
var gkReaction = _moBiProject.ReactionBlockCollection.First().First();
var glucosePath = gkReaction.Formula.ObjectPaths.ElementAt(1);
glucosePath.Last().ShouldBeEqualTo("Concentration");
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the constant instead of a magic screen

}

[Observation]
public void ShouldParseUserDefinedFunctions()
Copy link
Member

Choose a reason for hiding this comment

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

should_parse_user_defined_functions

@abdelr abdelr requested a review from msevestre October 7, 2021 14:44
@msevestre msevestre merged commit 0547048 into develop Oct 7, 2021
@msevestre msevestre deleted the 632_import_concentration_based_sbml_files branch October 7, 2021 15:24
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.

When importing SBML Files, respect when the description is concentration based
3 participants