Skip to content
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.

Sign up for GitHub

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

Jump to bottom

mock_slave violates assumptions, causing tests to be ill-defined #538

Closed
kyllingstad opened this issue Mar 16, 2020 · 1 comment
Closed

mock_slave violates assumptions, causing tests to be ill-defined #538

kyllingstad opened this issue Mar 16, 2020 · 1 comment
Assignees
@kyllingstad
Labels
bug Something isn't working

Comments

@kyllingstad
Copy link
Member

kyllingstad commented Mar 16, 2020 •
edited
Loading

While we haven't documented it explicitly, I think it's safe to say that CSE slaves should adhere to the same mathematical model as FMI slaves. In simplified terms, this is:

v(t) = f(t, x(t))
y(t) = g(t, v(t), x(t))

where x are input variables and parameters, v are the slave's internal states, and y are its outputs. In words: A slave's internal state is a function of time and input, while its output is a function of time, internal state and input.

mock_slave violates this by having its internal state depend on how many times the step function gets called, irrespective how much time passes. So I could call its do_step() function 10 times with Δt=0 and get the same result as if I called it 10 times with Δt=1.

I am afraid that this has a detrimental effect on the quality of our tests, because it means we tailor a lot of them to the behaviour of a misbehaving slave. (Worst case, we actually end up building wrong behaviour into CSE Core, though the existence of tests that involve external FMUs will of course help to prevent this.)
@kyllingstad kyllingstad added the bug Something isn't working label Mar 16, 2020
@kyllingstad kyllingstad self-assigned this Mar 16, 2020
@kyllingstad
Copy link
Member Author

I discovered this while implementing a test that expected correct behaviour from mock_slave. When I noticed the error and went to fix it, I ended up breaking a ton of other tests. So we definitely have a real problem with our test suite.

kyllingstad added a commit that referenced this issue Mar 16, 2020
@kyllingstad
Fix mock_slave and lots of magic numbers
dc0c527 
This fixes issue #538, "`mock_slave` violates assumptions, causing
tests to be ill-defined".  Basically, what I've done is to make the
user-supplied functions act on the outputs rather than on the internal
states.

This had quite substantial effects on some of the tests, which depended
on the wrong behaviour.  To compensate and, to some extent, restore
the old behaviour in a "correct" way, I've had to add some new
functionality to `mock_slave`, namely:

- The possibility to have outputs depend on time as well as inputs.
- The possibility to have a custom function called at each time step.

Finally, I've taken this opportunity to make some of the tests slightly
more readable by replacing magic numbers with named variables/constants.
This includes:

- Simulator indexes, which were assumed to be sequential in some tests
- Variable references, which were assumed to be 0 and 1 for
  `mock_slave`'s variables

The magic numbers were making the tests hard to read, and therefore also
to fix.  Besides, while the above assumptions are correct at the moment,
that need not be the case in the future.
@kyllingstad kyllingstad mentioned this issue Mar 16, 2020
Merged
kyllingstad added a commit that referenced this issue Mar 25, 2020
@kyllingstad
Fix mock_slave and lots of magic numbers (#539)
78a0bce 
This fixes issue #538, "`mock_slave` violates assumptions, causing
tests to be ill-defined".  Basically, what I've done is to make the
user-supplied functions act on the outputs rather than on the internal
states.

This had quite substantial effects on some of the tests, which depended
on the wrong behaviour.  To compensate and, to some extent, restore
the old behaviour in a "correct" way, I've had to add some new
functionality to `mock_slave`, namely:

- The possibility to have outputs depend on time as well as inputs.
- The possibility to have a custom function called at each time step.

Finally, I've taken this opportunity to make some of the tests slightly
more readable by replacing magic numbers with named variables/constants.
This includes:

- Simulator indexes, which were assumed to be sequential in some tests
- Variable references, which were assumed to be 0 and 1 for
  `mock_slave`'s variables

The magic numbers were making the tests hard to read, and therefore also
to fix.  Besides, while the above assumptions are correct at the moment,
that need not be the case in the future.
@ljamt ljamt closed this as completed May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kyllingstad kyllingstad

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@kyllingstad @ljamt