Add concentration units (mixtures/solutions) #646
Conversation
Added the following Concentration Units: - MassConcentration: SI = kg/m3, typically mg/l - VolumeConcentration : dimensionless, typically % - MassFraction: SI = kg/kg, typically mg/kg Modified the existing Molarity unit: - Some operations that were originally based on the Density units now use the MassConcentration units instead (Note: despite the fact that they share the same measurement units- the Density is a distnct QuantyType from the MassConcentration) - Removed all operators involving Molarity from the Density units Defined some basic operations that were missing from the AmountOfSubstance/MolarMass/Mass units Defined the triangular operations involving Mass/Molar/Volume concentrations (& the corresponding component's Density & MolarMass) All unit tests included most were defined by actual chemists(which I AM NOT). Note: one of the tests (QuantityIFormattableTests.VFormatEqualsValueToString)- was failing on my machine- it passes if I add CultureInfo.CurrentUICulture to the value.ToString() - as I presume was the intended behavior
|
Thanks for contributing, I'm a bit busy these days but me or @tmilnthorp will get around to review it soon. |
There was a problem hiding this comment.
I have reviewed the JSON files now, leaving the rest for now until we conclude on some discussions first.
Awesome job by the way! Pretty much perfectly following the conventions.
The only thing I would advise you for next time is to do smaller pull requests. It will take much less time to review and complete in smaller chunks :-)
- updated liter abbreviations for g/l, g/dl, g/ml & kg/l to uppercase 'L' (TODO Density?) - added base units to all units in MassConcentration & Molarity (TODO Density?)
- corrected the BaseUnits for MassConcentration - marked the invalid methods from Molarity/Density as Obsolete (were previously omitted) - some cosmetic changes to the Unit Tests
- MolesPerLiter: fixed the BaseUnits (default) to Deimeter/Mole - Molar: removed in favor of using the alternative abbreviation 'M" - MolarityTests - OneMilliMolarFromStringParsedCorrectly skipped while awaiting fix for #344
- added a KnownQuantities class with a few constants that were used in multiple tests - replaced the usages in MassConcentrationTests MolarityTests * VolumeConcentrationTests
- converted two of the MassConcentration tests to using Theory + InlineData
- removed BaseUnits from GramPerDeciliter(not exact + overlap), kept it in GramPerLiter (as exact & non-overlapping), also kept it for GramPerMilliliter(exact + overlapping) because I thought it would be useful to have at least one such case for future testing - moved the Mass/MolarMass operator to the Mass class (removing the MolarMass.extra) - all tests refactored using Theory + Inline Data - moved one or two tests to the appropriate .Test file - removed a few redundant tests
|
Very nice, I'm at the cabin this weekend and may not have the opportunity to look at this before Sunday. |
| [Fact] | ||
| public void HClSolutionVolumeIsEqualToExpected() | ||
| [Theory] | ||
| [InlineData(5, MassUnit.Gram, |
There was a problem hiding this comment.
To be clear, this is fine, but I didn't mean that [InlineData] was my preferred method of testing. Only that the values would be inlined/inserted into the test code itself, so that you can read all the numbers that go into the calculation for each test method rather than reusing fields. [Fact] is probably more "correct" than [Theory] + [InlineData] if there is only one test case. If you have multiple test cases/inputs, then [InlineData] is a good fit.
At any rate, this works and I'm not going to bother you to change it again.
angularsen
changed the title
Added the following Concentration Units:
Modified the existing Molarity unit (AKA Molar Concentration):
Defined some basic operations that were missing from the AmountOfSubstance/MolarMass/Mass units
Defined the triangular operations involving Mass/Molar/Volume concentrations (using the solute's Density and/or Molar Mass)
All unit tests included, most were defined by actual chemists (which I AM NOT).
Note: one of the tests (QuantityIFormattableTests.VFormatEqualsValueToString)- was failing on my machine- it passes if I add CultureInfo.CurrentUICulture to the value.ToString() - as I presume was the intended behavior