Skip to content

adding unit conversion functions#175

Merged
jonc125 merged 54 commits intodevelopfrom
77-unit-conversion
Jan 8, 2020
Merged

adding unit conversion functions#175
jonc125 merged 54 commits intodevelopfrom
77-unit-conversion

Conversation

@skeating
Copy link
Copy Markdown
Contributor

@skeating skeating commented Dec 5, 2019

Fixes #77

Description

Adding functions necessary for unit conversion.

  • take a variable and unit and convert all relevant values/equations in model to reflect new unit on variable
  • add_input function
  • add_output function
  • model missing units - MOVED to issue Add a custom unit to registry as a Unit object #192
  • arguments as strings/objects
  • review arg checking
  • refactor is_output to not be in API
  • test state symbols are correct after an input is added
  • test substitution of rhs deriv recurses into sub expressions
  • add helper function for getting unique name
  • check _convert_variable_instance need not replace deriv for output (step 4)
  • formatting of docstring examples
  • change function name/args/add enum

Motivation and Context

Fixes #77

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)

Checklist:

  • I have updated all documentation necessary.
  • I have checked spelling in (new) comments.

Testing

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 5, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@bd8411f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #175   +/-   ##
==========================================
  Coverage           ?   96.14%           
==========================================
  Files              ?        6           
  Lines              ?      987           
  Branches           ?      208           
==========================================
  Hits               ?      949           
  Misses             ?       17           
  Partials           ?       21

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd8411f...6cc0e0c. Read the comment docs.

@MichaelClerx
Copy link
Copy Markdown
Contributor

This is looking good!

@skeating
Copy link
Copy Markdown
Contributor Author

Requesting review of whats here.
Questions:

  1. how do I distinguish between input/output if I wanted to create a generic convert(some_variable, some_units) and it figured out which to call ?
  2. should I make it so that arguments are either string/DummyVariable and string/Unit object ?
  3. any ideas why appveyor is failing - tests pass locally
  4. I know I need to deal with case where the model does not have the required units in its registry

Comment thread cellmlmanip/model.py
@skeating
Copy link
Copy Markdown
Contributor Author

skeating commented Jan 3, 2020

Asking for review of this as it stands. With hope to merge with following questions:

  1. Do I need to fiddle and get appveyor to pass

  2. I could change current convert_variable so that the unit argument could also be a name/attributes pair which would allow it to be added if necessary while I work on Add a custom unit to registry as a Unit object #192

@skeating skeating dismissed MichaelClerx’s stale review January 3, 2020 14:12

Naming is sorted and Michael is away for a month

@skeating skeating requested a review from jonc125 January 3, 2020 14:13
@jonc125
Copy link
Copy Markdown
Contributor

jonc125 commented Jan 3, 2020

Asking for review of this as it stands. With hope to merge with following questions:

1. Do I need to fiddle and get appveyor to pass

No, sympy 1.5 support should be addressed in a separate PR.

2. I could change current convert_variable so that the unit argument could also be a name/attributes pair which would allow it to be added if necessary while I work on #192

I don't think we want to do this.

@skeating
Copy link
Copy Markdown
Contributor Author

skeating commented Jan 3, 2020

Sorry @jonc125 just saw your post on slack. Code at present works if unit to be converted-to is in same registry as converted-from so it covers your suggestion. Weblab-fc would be place to take care of protocol/model using same registry and that I'll cover with taking over Michael's issue. I'll leave the other issue open and think a bit more.

Copy link
Copy Markdown
Contributor

@jonc125 jonc125 left a comment

Choose a reason for hiding this comment

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

With two exceptions I think these are just cosmetic/documentation comments. The more substantive items are:

  1. After seeing the code, I think it'll actually make our lives easier to say convert_variable has to be passed a VariableDummy.
  2. When converting we ought to delete the initial_value from the original variable if it's moved over, as it gets defined by an equation instead.

Comment thread cellmlmanip/model.py Outdated
Comment thread cellmlmanip/model.py Outdated
Comment thread cellmlmanip/model.py Outdated
Comment thread cellmlmanip/model.py Outdated
Comment thread cellmlmanip/model.py Outdated
Comment thread tests/test_unit_conversion.py Outdated
Comment thread tests/test_unit_conversion.py Outdated
Comment thread tests/test_unit_conversion.py
Comment thread tests/test_unit_conversion.py Outdated
Comment thread tests/test_unit_conversion.py Outdated
@skeating skeating requested a review from jonc125 January 6, 2020 17:32
Copy link
Copy Markdown
Contributor

@jonc125 jonc125 left a comment

Choose a reason for hiding this comment

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

Looks good. Just one typo and one minor fix spotted!

Comment thread cellmlmanip/model.py
Comment thread cellmlmanip/model.py Outdated
assert isinstance(variable, VariableDummy)

# units should be a pint Unit object
# variable must b in model
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/b/be/

@jonc125 jonc125 merged commit 00403b4 into develop Jan 8, 2020
@jonc125 jonc125 deleted the 77-unit-conversion branch January 8, 2020 14:18
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.

Adding unit conversion for code generation

4 participants