Demonstrating errors in code generation #598

nickerso · 2020-05-12T10:28:52Z

I have added code generation for my "go to" test model (sine approximations with imports) to demonstrate an issue that I am seeing in the code generation. There is some evidence that this is related to #597, but as well as the ordering being incorrect, the VARIABLE_INFO is getting the name of the variables wrong. While the variable info correctly identifies the "source" component, it is using the name of the variable from the top-level component in the encapsulation and import hierarchy.

You can see these differences in specifically in nickerso@41db32b. This is a problem in both the C and Python profiles.

Also need to update the expected printer string in the tests to reflect the additional variable and connection.

The added test will pass, but the generated variable information is incorrect. Adding this version to show what is currently being generated.

…cal value

This change breaks the tests as the code being generated is incorrect. Also looks like the ordering is incorrect (variables[2] corresponds to the variable information for index 7). See cellml#597.

agarny · 2020-05-12T10:41:45Z

Good catch! FWIW, I would indeed expect the behaviour to be the same indepdent on the profile chosen. Anyway, it should be an easy fix. I will do it tomorrow once PRs #596 and #599 have been merged in.

nickerso · 2020-05-12T10:46:17Z

once PRs #596 and #599 have been merged in

Surely the fix can be done without a merge...this is completely blocking me at present...I'm building my own libcellml so not worried about changes being merged in...pretty please :)

agarny · 2020-05-13T00:46:17Z

I think the problem (besides issue #597) is not with the generator itself, but rather with the flattening.

agarny · 2020-05-13T01:08:36Z

I think the problem (besides issue #597) is not with the generator itself, but rather with the flattening.

I take it back. It is indeed an issue with the generator. Argh!

nickerso · 2020-05-13T01:35:58Z

I take it back. It is indeed an issue with the generator.

phew! was pretty sure I checked the flattened model and it looked good. This issue came up as I try to navigate through the collection of equivalent variables to find the one that is used in the generated code and I can never get a match as the component and variable names are out of sync.

agarny · 2020-05-13T01:38:45Z

I take it back. It is indeed an issue with the generator.

phew! was pretty sure I checked the flattened model and it looked good. This issue came up as I try to navigate through the collection of equivalent variables to find the one that is used in the generated code and I can never get a match as the component and variable names are out of sync.

Yes, I printed the flattened model and that's where I realised my mistake. :) The problem is buried in our generator, but I am slowly getting there, so you will have a fix before the end of the day. Definitely.

…neration.

…e one in which its equation is.

…me as the equation variable.

agarny · 2020-05-13T08:58:19Z

Ok, I am done with this. Happy for you, @nickerso, @hsorby and @kerimoyle, to review and merge this in.

agarny · 2020-05-13T08:59:11Z

@nickerso, since you can't review it as such. I am happy to "review" it on your behalf...

nickerso · 2020-05-13T09:49:58Z

Mild panic as this didn't fix my issue, but then realised that I have the ordering fix from #597 also merged into my local source. So ignoring that, yes, this fixes the bug in the variable names and I would happily approve this if I hadn't started it :)

agarny · 2020-05-13T10:58:28Z

Eh, it's not the time for mild panic! :) Anyway, I am going to approve it on your behalf and leave it to @hsorby or @kerimoyle to confirm.

agarny

This is a brilliant PR. I am all in favour of it. No reservations whatsoever! (If only all PRs could be of that quality. One can only dream...)

kerimoyle · 2020-05-13T19:15:41Z

src/generator.cpp

        // The variable has an initial value, so it can either be a constant or
        // a state. By default, we consider it to be a constant and, if we find
        // an ODE for that variable, we will know that it was actually a state.

        mType = Type::CONSTANT;
+
+        mInitialValueVariable = variable;


A variable that is reset will have an initial value too? Neither a constant nor a state?

We are not yet generating code for resets, so we will see what needs to be done when we do.

kerimoyle

Which is the bit that fixes the bug? There are lots of changes that seem a bit arbitrary?

tests/resources/deriv_approx_sin.xml

agarny · 2020-05-13T19:54:44Z

Which is the bit that fixes the bug? There are lots of changes that seem a bit arbitrary?

This PR contains some code clean up (as a result of working on this PR), but otherwise there are a couple of key places that fix the bug. I am going to annotate them.

agarny · 2020-05-13T20:03:34Z

src/generator.cpp

-        variable->mComponent = mComponent;
+        for (size_t i = 0; i < mComponent->variableCount(); ++i) {
+            VariablePtr localVariable = mComponent->variable(i);
+
+            if (sameOrEquivalentVariable(variable->mVariable, localVariable)) {
+                variable->setVariable(localVariable, false);
+
+                break;
+            }
+        }


This is the change that directly fixes @nickerso's issue. Before we were only updating the component information, but not the variable information which we now do through our call to setVariable(). Note, however, that we pass it false since we don't want to check for the initial value.

agarny · 2020-05-13T20:03:39Z

src/generator.cpp

-void GeneratorInternalVariable::setVariable(const VariablePtr &variable)
+void GeneratorInternalVariable::setVariable(const VariablePtr &variable,
+                                            bool checkInitialValue)
 {
-    mVariable = variable;
-
-    if (!variable->initialValue().empty()) {
+    if (checkInitialValue && !variable->initialValue().empty()) {
        // The variable has an initial value, so it can either be a constant or
        // a state. By default, we consider it to be a constant and, if we find
        // an ODE for that variable, we will know that it was actually a state.

        mType = Type::CONSTANT;
+
+        mInitialValueVariable = variable;
    }
+
+    mVariable = variable;
+    mComponent = std::dynamic_pointer_cast<Component>(variable->parent());
 }


Our new setVariable() method now also updates mVariable, but also keeps track of the variable object that contains the initial value of a variable (mInitialValueVariable).

agarny · 2020-05-13T20:03:51Z

src/api/libcellml/generator.h

-     * variable). It may or may not be the same @c Component as the parent
-     * component of the @c Variable.
+     * variable).


Because of points 1 and 2, the component() is always going to be the parent component of variable(), which makes our API better than before.

agarny · 2020-05-13T20:03:57Z

src/generator.cpp

-    return mProfile->indentString() + generateVariableNameCode(variable->mVariable) + " = " + generateDoubleCode(variable->mVariable->initialValue()) + mProfile->commandSeparatorString() + "\n";
+    return mProfile->indentString() + generateVariableNameCode(variable->mVariable) + " = " + generateDoubleCode(variable->mInitialValueVariable->initialValue()) + mProfile->commandSeparatorString() + "\n";


This is a side effect of the main change where we now have to retrieve the initial value from mInitialValueVariable rather than mVariable.

kerimoyle · 2020-05-13T20:36:35Z

I've got some concerns about the "can't initialise a variable with another variable" thing. We've just finished writing the spec that says that we do support this (at present) ... shouldn't we be maintaining tests and functionality in line with that? I have no problem with this PR other than this sneaking idea that we might be missing stuff by making changes in the background ... ? @nickerso @hsorby @agarny Thoughts?

nickerso · 2020-05-13T21:34:54Z

I've got some concerns about the "can't initialise a variable with another variable" thing

CellML allows this, the develop branch of libCellML does not yet support this for code generation. As code generation support for this improves this will be added back in. For this particular test I used the existing sine approximations model to save time, happy for the PR to be updated to use a different model to demonstrate this issue (which I think there were others that @agarny fixed as part of this so they could be sufficient).

kerimoyle

OK with me.

agarny · 2020-05-13T21:42:55Z

I've got some concerns about the "can't initialise a variable with another variable" thing. We've just finished writing the spec that says that we do support this (at present) ... shouldn't we be maintaining tests and functionality in line with that?

This needs to be done in another PR for which I have created an issue (see issue #602). Will work on that issue once we have merged in this PR and PR #599.

nickerso added 4 commits May 12, 2020 22:06

Add missing parameter defition into the sine approximations test model.

e95816e

Also need to update the expected printer string in the tests to reflect the additional variable and connection.

Adding code generation of the sine approximation with imports model.

8a61e3b

The added test will pass, but the generated variable information is incorrect. Adding this version to show what is currently being generated.

replace the not-yet-supported variable as initial_value with a numeri…

793ca1e

…cal value

correct VARIABLE_INFO with the actual variable names.

41db32b

This change breaks the tests as the code being generated is incorrect. Also looks like the ordering is incorrect (variables[2] corresponds to the variable information for index 7). See cellml#597.

agarny mentioned this pull request May 12, 2020

Make Clang-Tidy happy #596

Merged

Some minor cleaning up.

8cc8539

agarny added 8 commits May 13, 2020 13:46

Merge remote-tracking branch 'upstream/develop' into checking-code-ge…

91414ce

…neration.

Some constant variables were not reported in the correct component.

068976e

Some minor cleaning up.

cdcfedf

Some minor cleaning up.

6156f64

Some minor cleaning up.

c0dec7d

Generator: update the variable of an internal variable to point to th…

28043d6

…e one in which its equation is.

Generator: small fix in case the initial value variable is not the sa…

3728fcd

…me as the equation variable.

Some minor cleaning up.

befd3c8

agarny requested review from agarny, hsorby and kerimoyle May 13, 2020 08:58

agarny approved these changes May 13, 2020

View reviewed changes

kerimoyle reviewed May 13, 2020

View reviewed changes

tests/resources/deriv_approx_sin.xml Show resolved Hide resolved

agarny reviewed May 13, 2020

View reviewed changes

kerimoyle approved these changes May 13, 2020

View reviewed changes

agarny merged commit 01cc7ad into cellml:develop May 13, 2020

agarny mentioned this pull request May 20, 2020

Code generation for connections that use compatible, but not equivalent, units (i.e. need to be scaled) #572

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demonstrating errors in code generation #598

Demonstrating errors in code generation #598

nickerso commented May 12, 2020

agarny commented May 12, 2020

nickerso commented May 12, 2020

agarny commented May 13, 2020

agarny commented May 13, 2020

nickerso commented May 13, 2020

agarny commented May 13, 2020

agarny commented May 13, 2020

agarny commented May 13, 2020

nickerso commented May 13, 2020

agarny commented May 13, 2020 •

edited

Loading

agarny left a comment

kerimoyle May 13, 2020

agarny May 13, 2020

kerimoyle left a comment

agarny commented May 13, 2020

agarny May 13, 2020

agarny May 13, 2020

agarny May 13, 2020

agarny May 13, 2020

kerimoyle commented May 13, 2020

nickerso commented May 13, 2020

kerimoyle left a comment

agarny commented May 13, 2020

		return mProfile->indentString() + generateVariableNameCode(variable->mVariable) + " = " + generateDoubleCode(variable->mVariable->initialValue()) + mProfile->commandSeparatorString() + "\n";
		return mProfile->indentString() + generateVariableNameCode(variable->mVariable) + " = " + generateDoubleCode(variable->mInitialValueVariable->initialValue()) + mProfile->commandSeparatorString() + "\n";

Demonstrating errors in code generation #598

Demonstrating errors in code generation #598

Conversation

nickerso commented May 12, 2020

agarny commented May 12, 2020

nickerso commented May 12, 2020

agarny commented May 13, 2020

agarny commented May 13, 2020

nickerso commented May 13, 2020

agarny commented May 13, 2020

agarny commented May 13, 2020

agarny commented May 13, 2020

nickerso commented May 13, 2020

agarny commented May 13, 2020 • edited Loading

agarny left a comment

Choose a reason for hiding this comment

kerimoyle May 13, 2020

Choose a reason for hiding this comment

agarny May 13, 2020

Choose a reason for hiding this comment

kerimoyle left a comment

Choose a reason for hiding this comment

agarny commented May 13, 2020

agarny May 13, 2020

Choose a reason for hiding this comment

agarny May 13, 2020

Choose a reason for hiding this comment

agarny May 13, 2020

Choose a reason for hiding this comment

agarny May 13, 2020

Choose a reason for hiding this comment

kerimoyle commented May 13, 2020

nickerso commented May 13, 2020

kerimoyle left a comment

Choose a reason for hiding this comment

agarny commented May 13, 2020

agarny commented May 13, 2020 •

edited

Loading