fixing errors deling with offsets in unit definitions

[MauriceHendrix]

Description

Fixes errors dealing with:

• offsets in unit definitions (warning if non-0 and not dimensionless)
• offsets in demenionless units (error)
• demenionless units with an exponent
• demenionless units with a multiplier

Motivation and Context

fixes #351

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Documentation checklist

• I have updated all documentation in the code where necessary.
• I have checked spelling in all (new) comments and documentation.
• I have added a note to RELEASE.md if relevant (new feature, breaking change, or notable bug fix).

Testing

• Testing is done automatically and codecov shows test coverage
• This cannot be tested automatically

[codecov]

Codecov Report

Merging #354 (83bb0b5) into master (9bf67f7) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #354   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines         1564      1569    +5     
  Branches       371       373    +2     
=========================================
+ Hits          1564      1569    +5     

Impacted Files	Coverage Δ
cellmlmanip/parser.py	100.00% <100.00%> (ø)
cellmlmanip/units.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[MichaelClerx]

The offset bit won't work. Offsets are supposed to change the unit, which pint doesn't support, so we don't implement it in cellmlmanip (see here: #8)

The presence of an offset that isn't 0 should be a warning or an error for all units (dimensionless or not)

[MauriceHendrix]

now throwing error for dimentionless + offset other non-0 offsets

[MichaelClerx]

[michael@fedora cellmlmanip]$ grep "upported" ./cellmlmanip/ -Ir
./cellmlmanip/units.py:# The full list of supported CellML units
./cellmlmanip/units.py:            raise ValueError('Unit <%s> is not currently supported by cellmlmanip.' % name)
./cellmlmanip/units.py:            raise KeyError('Unit <' + str(name) + '> is not currently supported by cellmlmanip.')
./cellmlmanip/units.py:        :raises UnexpectedMathUnitsError: if math is not supported
./cellmlmanip/units.py:        self.message = message or 'The math used by this expression is not supported.'
./cellmlmanip/parser.py:                    'Defining units inside components is not supported (found in component ' + name + ').')
./cellmlmanip/parser.py:                    'Reactions are not supported (found in component ' + name + ').')
./cellmlmanip/parser.py:            raise ValueError('Invalid or unsupported CellML file. ' + msg)
./cellmlmanip/model.py:                    'Non-local annotations are not supported.')
./cellmlmanip/model.py:        Note that only cmeta ids on variables or the model itself are checked and supported.
./cellmlmanip/model.py:                raise ValueError('Only first order derivatives wrt a single variable are supported')
./cellmlmanip/printer.py:            'Unsupported expression type (' + str(type(expr)) + '): '
./cellmlmanip/printer.py:        raise ValueError('Unsupported function: ' + str(name))
./cellmlmanip/printer.py:            raise ValueError('Unsupported relational: "' + str(op) + '".')

looks like we just use ValueError everywhere else where something isn't supported. Should probably be the same here? Also not sure why we need special logic for dimensionless-with-offset ? Any why pass the offset all the way through instead of raising it in the parser?

[MauriceHendrix]

aah sorry I thought you wanted to make a distinction between dimentionless + offset and non-dimensionless + non-0 offset :) I've changed it to throw ValueError for non-0 offsets and indeed no need to carry the offset.

[MichaelClerx]

I don't understand what this code does, at all!
Could you add some comments to explain it?

[MauriceHendrix]

Indeed it looks like a pint error, you can convert one way, but not the other way.
actually I think it should be quantity.magnitude / quantity.to(unit) rather than 1 / quantity.to(unit)
so that 2 meters become 4 half meters en 3 half meters become 1.5 meter i.e.

    def test_dimensionless_multiplier2(self):
        model = load_model('dimensionless_multiplier2.cellml')
        meter = model.units.get_unit('meter')
        half_meter = model.units.get_unit('half_meter')
        cf1 = model.units.convert(2 * meter, half_meter).magnitude
        cf2 = model.units.convert(3 * half_meter, meter).magnitude
        assert cf1 == 4.0
        assert cf2 == 1.5

[MichaelClerx]

Just one question left