WIP Fixes failign build #625

msevestre · 2021-10-04T16:05:06Z

@abdelr I tried to fix the code but I have no idea what's going on .Why aren't you using the capability of the internal unit system to convert from one unit to another? If feels that you are multiplying TWICE with the rate when you care converting.
I have run resharper on the file (it would be awesome if you could run it as well) to simplify the code and I also implemented all the suggestion.
Then I removed the usage of two dimensionFactory in the same file (they should be the same instance anyways). I am sure you can figure out what's going on

msevestre · 2021-10-04T16:05:31Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

         Rate *= multiplier * Math.Pow(10, scale);
      }
-      private Unit _direct { get; set; }


private menber. Why a get set?

Not really needed, sometimes I do this for finding easier where it is been used. So even when it is private, it is a full property and you can easily navigate but you are right, not necessary.

Resharper navigates with private members...plus it follows the rest of the code

msevestre · 2021-10-04T16:06:05Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

@@ -48,37 +53,39 @@ public class UnitDefinitionImporter : SBMLImporter, IStartable, IUnitDefinitionI
   {
      private readonly IMoBiDimensionFactory _moBiDimensionFactory;
      private readonly IDictionary<int, Unit> _baseUnitsDictionary;
-      private readonly IDictionary<string, UnitConvertionInfo> _unitConvertionDictionary;
-      private IDimensionFactory _dimensionFactory;


why this guy here sinec we have a MoBiDimensionFactory (which is a specification of IDimensionFActory)

msevestre · 2021-10-04T16:06:33Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

      {
         _baseUnitsDictionary.Add(libsbml.UNIT_KIND_AMPERE, _moBiDimensionFactory.Dimension("Ampere").Unit("A"));
         _baseUnitsDictionary.Add(libsbml.UNIT_KIND_BECQUEREL, _moBiDimensionFactory.Dimension("Becquerel").Unit("Bq"));
         _baseUnitsDictionary.Add(libsbml.UNIT_KIND_CANDELA, _moBiDimensionFactory.Dimension("Candela").Unit("cd"));
         _baseUnitsDictionary.Add(libsbml.UNIT_KIND_COULOMB, _moBiDimensionFactory.Dimension("Coulomb").Unit("C"));
-         _baseUnitsDictionary.Add(libsbml.UNIT_KIND_DIMENSIONLESS, _moBiDimensionFactory.NoDimension.DefaultUnit);
+         _baseUnitsDictionary.Add(libsbml.UNIT_KIND_DIMENSIONLESS, _moBiDimensionFactory.NoDimension.BaseUnit);


I am pretty sure you mean BaseUnit and not DefaultUnit. We always need to convert to BaseUnit.

msevestre · 2021-10-04T16:06:44Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

@@ -131,7 +140,7 @@ public IDimension ConvertUnit(UnitDefinition unitDefinition)
         if (dimension != Constants.Dimension.NO_DIMENSION)
         {
            _sbmlInformation.MobiDimension[sbmlUnit] = dimension;
-            _unitConvertionDictionary.Add(sbmlUnit, new UnitConvertionInfo() { Dimension = dimension, Unit = dimension.DefaultUnit, Rate = 1 });
+            _unitConversionDictionary.Add(sbmlUnit, new UnitConversionInfo {Dimension = dimension, Unit = dimension.BaseUnit, Rate = 1});


I am pretty sure you mean BaseUnit and not DefaultUnit. We always need to convert to BaseUnit.

msevestre · 2021-10-04T16:06:53Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

            return dimensionAndUnit.dimension;
         }

-         _unitConvertionDictionary.Add(sbmlUnit, new UnitConvertionInfo() { Dimension = dimension, Unit = dimension.DefaultUnit, Rate = 1 });
+         _unitConversionDictionary.Add(sbmlUnit, new UnitConversionInfo {Dimension = dimension, Unit = dimension.BaseUnit, Rate = 1});


I am pretty sure you mean BaseUnit and not DefaultUnit. We always need to convert to BaseUnit.

msevestre · 2021-10-04T16:07:21Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

-            var convertionData = _unitConvertionDictionary[unit];
-            return (convertionData.Dimension.UnitValueToBaseUnitValue(convertionData.Unit, value * convertionData.Rate), convertionData.Dimension);
+            var conversionData = _unitConversionDictionary[unit];
+            return (conversionData.Dimension.UnitValueToBaseUnitValue(conversionData.Unit, value * conversionData.Rate), conversionData.Dimension);


Why do we need this rate? It feels that yo are converting twice since we are also converting using the value

@msevestre There are indeed two conversions. The first conversion is based on the description on the sbml file. The second is to the mobi base unit. Suppose there is a description from unit A to a (sbml) base unit B described in the file. This will be captured by the rate. Unit A does not exist on mobi but unit B does. Additionally, unit B needs to be translated to mobi base unit C. More specifically, we have an entry on the sbml file that specifies that "unit A" is going to be used and it has a given rule to be translated to "mole" (which is a conventional unit known in mobi too). This translation is captured in rate. But when we translate in mobi, we actually need to translate from "unit A" to "micro mole" which is the base unit, so we need to translate from "unit A" to "mole" and then to "micro mole". Does it make sense? It is not a clean explanation but we could talk about this tomorrow if you like. I will take a look on the changes though and also on the failing test.

msevestre · 2021-10-04T16:08:03Z

tests/MoBi.Tests/Core/SBML/UnitImporterSpecs.cs

+      public void NewDimensionsTests()
+      {
+         sut.ToMobiBaseUnit("substance", 3e6).value.ShouldBeEqualTo(3e12);
+         sut.ToMobiBaseUnit("inverse_day", 0.83).value.ShouldBeEqualTo(0.83 / 1440); //base unit is in minutes so 1440 minutes in one day


but isn't the value in second based on _unit3?

Minutes is the base unit, right?

msevestre · 2021-10-04T16:08:56Z

tests/MoBi.Tests/Core/SBML/UnitImporterSpecs.cs

@@ -117,7 +100,7 @@ protected override void Context()
      [Observation]
      public void ParameterWithVolumeUnitTest()
      {
-         var tc = _moBiProject.SpatialStructureCollection.FirstOrDefault().TopContainers.FirstOrDefault();
+         var tc = _moBiProject.SpatialStructureCollection.First().TopContainers.First();


It should always be First since you are calling something after. FirstOrDefault does not make sense. Probably old code but we might as well fix it

Sure, you are right.

msevestre · 2021-10-04T16:09:37Z

@abdelr The test is still failing. Just some cleanup

msevestre

I think I found the issue. The factor was wrong. I have also added some comments. Please let me know if they are accurate

msevestre · 2021-10-04T20:13:02Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

         Rate *= multiplier * Math.Pow(10, scale);
      }
-      private Unit _direct { get; set; }


Resharper navigates with private members...plus it follows the rest of the code

msevestre · 2021-10-04T20:14:42Z

tests/MoBi.Tests/Core/SBML/UnitImporterSpecs.cs

+
+         _unit3 = _sbmlModel.createUnit();
+         _unit3.setExponent(-1);
+         _unit3.setMultiplier(1 / 86400.0); //86400 seconds in one day


@abdelr This was wrong I think. The way I understand the code. This is the factor to go from 1/ day to 1/s
so it should be 1/86400 asd not 86400 correct?

msevestre · 2021-10-04T20:15:14Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

   {
      public IDimension Dimension { get; set; }
+
+      //Unit in which the values are stored in SBML Base unit
      public Unit Unit { get; set; }


@abdelr Comment here? Is this correct?

msevestre · 2021-10-04T20:16:10Z

src/MoBi.Engine/Sbml/UnitDefinitionImporter.cs

+      public string Name => (_direct?.Name ?? "1") + (_inverse != null ? $"/{_inverse.Name}" : "");
+
+      // Rate is the factor to convert from the unit used in sbml to the base unit used in sbml. 
+      // This is not necessarily the base unit in the ospsuite (1/s vs 1/min for inversed day for instance)
      public double Rate { get; private set; } = 1;


@abdelr Comment here. Can you verify that this is correct?

msevestre · 2021-10-05T13:30:39Z

@abdelr I forgot to publish my comment yesterday. Since you approved, I am assuming it's fine

abdelr · 2021-10-05T18:46:28Z

@msevestre Yes, I looked into the changes and made perfect sense for me.

WIP Fixes failign build

6934b64

msevestre requested a review from abdelr October 4, 2021 16:05

msevestre commented Oct 4, 2021

View reviewed changes

Fixes failing build

764a26b

msevestre commented Oct 4, 2021

View reviewed changes

abdelr approved these changes Oct 5, 2021

View reviewed changes

msevestre commented Oct 5, 2021

View reviewed changes

msevestre merged commit 20e3e5f into develop Oct 5, 2021

msevestre deleted the fixes-failing-build branch October 5, 2021 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Fixes failign build #625

WIP Fixes failign build #625

msevestre commented Oct 4, 2021

msevestre Oct 4, 2021

abdelr Oct 4, 2021

msevestre Oct 4, 2021

msevestre Oct 4, 2021

msevestre Oct 4, 2021

msevestre Oct 4, 2021

msevestre Oct 4, 2021

abdelr Oct 4, 2021

msevestre Oct 4, 2021

abdelr Oct 4, 2021

msevestre Oct 4, 2021

abdelr Oct 4, 2021

msevestre Oct 4, 2021

abdelr Oct 4, 2021

msevestre commented Oct 4, 2021

msevestre left a comment

msevestre Oct 4, 2021

msevestre Oct 4, 2021

msevestre Oct 4, 2021

msevestre Oct 4, 2021

msevestre commented Oct 5, 2021

abdelr commented Oct 5, 2021

WIP Fixes failign build #625

WIP Fixes failign build #625

Conversation

msevestre commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre commented Oct 4, 2021

msevestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre commented Oct 5, 2021

abdelr commented Oct 5, 2021