ModellingWebLab / cellmlmanip

CellML loading and model equation manipulation
Other
3 stars 1 forks source link

fixing errors deling with offsets in unit definitions #354

Closed MauriceHendrix closed 2 years ago

MauriceHendrix commented 2 years ago

Description

Fixes errors dealing with:

Motivation and Context

fixes #351

Types of changes

Documentation checklist

Testing

codecov[bot] commented 2 years ago

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%> (ø)

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

MichaelClerx commented 2 years ago

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: https://github.com/ModellingWebLab/cellmlmanip/issues/8)

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

MauriceHendrix commented 2 years ago

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

MichaelClerx commented 2 years ago
[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 commented 2 years ago

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.