-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor amici.ode_export, add AmiciCxxCodePrinter #1610
Conversation
94afaef
to
8095d96
Compare
Move custom codeprinting to a separate code printer class and reuse the same instance for all printing. Avoids plenty of reinstantiations of the code printer and keeps all settings in one place.
8095d96
to
469c8cb
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1610 +/- ##
===========================================
+ Coverage 78.76% 78.80% +0.04%
===========================================
Files 67 68 +1
Lines 10683 10666 -17
===========================================
- Hits 8414 8405 -9
+ Misses 2269 2261 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 nice cleanup!
@@ -3114,7 +3027,7 @@ def _get_function_body(self, | |||
f'reinitialization_state_idxs.cend(), {index}) != ' | |||
'reinitialization_state_idxs.cend())', | |||
f' {function}[{index}] = ' | |||
f'{_print_with_exception(formula)};' | |||
f'{self.model._code_printer.doprint(formula)};' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it make more sense to have _code_printer
as member of the exporter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully agreed. The problem is that ODEModel also generates code (which I think should be changed, but maybe rather separately). Having it in ODEModel for now allows using it also in ODEExporter, but not vice versa.
Move custom codeprinting to a separate code printer class and reuse the same instance for all printing.
Avoids plenty of reinstantiations of the code printer and keeps all settings in one place.