Synchronised variable transfer #189

kyllingstad · 2019-02-25T14:33:09Z

This fixes issue #181, "Only transfer variable values at common communication points".

Please have a good look at this and give it some consideration before approving, as it's a bit difficult to verify with tests that this is the right way to go about it. (Fixing #186 would help.)

This fixes issue #181, "Only transfer variable values at common communication points".

eidekrist · 2019-02-26T07:42:51Z

Sounds like it would be quite straightforward to verify this functionality with a test? For example two mock slaves with decimation factors of 2 and 3, connect real out->real in, verify that values are transferred every 6 base steps. Or are you saying such a test would not be sufficient?

mrindar · 2019-02-26T08:27:06Z

What happens here if we have slaves: A, B and C, where slave A and B both have one output connected to an input of slave C, and all of them have different decimation factors? In this case I suppose we want to step slave C only when A, B and C is at a common communication point? Also, is there any reason to transfer variables if a step is not going to be perfomed?

kyllingstad · 2019-02-26T09:11:19Z

Sounds like it would be quite straightforward to verify this functionality with a test?

I was thinking more like verifying that it doesn't have subtle unwanted consequences for the simulation results, but you are right, just testing the basic functionality should be simple enough. And it should be done, so I'll add a test for it!

kyllingstad · 2019-02-26T09:17:15Z

What happens here if we have slaves: A, B and C, where slave A and B both have one output connected to an input of slave C, and all of them have different decimation factors? In this case I suppose we want to step slave C only when A, B and C is at a common communication point?

This has nothing to do with when to step, that is controlled solely by the step size and decimation factor of each individual slave. This only has to do with when we transfer variable values. And with the change proposed here, we transfer from A to C whenever they have a common communication point, and from B to C whenever they have a common communication point.

Also, is there any reason to transfer variables if a step is not going to be perfomed?

Not really, but it doesn't harm either, and it's easier to just update the variables than to special-case it. It probably won't affect performance that much, because it only updates the local cache. Nothing is actually sent to the slave until do_step() is called anyway.

mrindar · 2019-02-26T09:29:28Z

This has nothing to do with when to step, that is controlled solely by the step size and decimation factor of each individual slave. This only has to do with when we transfer variable values. And with the change proposed here, we transfer from A to C whenever they have a common communication point, and from B to C whenever they have a common communication point.

I suppose this is OK, but when would C be stepped in this scenario? It is my understanding that slave C should only be stepped at time t_k, when all the inputs have valid values for time t_k. If this is the case, then C should only be stepped at the time point which is common for all slaves conntected to its inputs, in this case slave A and B.

ssadjina · 2019-02-26T09:59:49Z

I think it helps to ask "when must a connection/bond step?" and not "when must a slave step?". It actually makes little sense IMHO to talk about when one slave should step/synchronize, it always is a matter of when a connection/bond should "step" and synchronize the slaves it connects.
So in @mrindar 's example I guess one should really define a step size for bond A-C and one for B-C and then follow the logic:

A steps when A-C steps.
B steps when B-C steps.
C steps whenever A-B or A-C steps.

Thoughts?

mrindar · 2019-02-26T10:10:58Z

3\. C steps whenever A-B or A-C steps.

This means that at a given doStep for C, the input values will not be synchronized for the given time point. Which can not possibly be correct(?)

kyllingstad · 2019-02-26T10:21:03Z

I suppose this is OK, but when would C be stepped in this scenario? It is my understanding that slave C should only be stepped at time t_k, when all the inputs have valid values for time t_k.

In my opinion, this is not the case. C should be stepped at the rate that the user has set for it. If there are no new values for one or more of its inputs, those inputs should simply not be set, and the slave should be free to handle this however it sees fit (e.g. by extrapolation from previous values).

I base this on two observations (or rather lack thereof) of things that are not mentioned in the FMI standard:

There is no rule in FMI stating that fmi2SetXxx() must be called for all inputs at all communication points.
It is not specified that the slave must use the old value for an input variable if it doesn't receive a new one.

kyllingstad · 2019-02-26T10:22:42Z

If there are no new values for one or more of its inputs, those inputs should simply not be set

Note that this is currently not what happens; the inputs always get set due to the way we do caching. This is what @hplatou mentioned in today's meeting.

kyllingstad · 2019-02-26T10:30:10Z

If there are no new values for one or more of its inputs, those inputs should simply not be set, and the slave should be free to handle this however it sees fit (e.g. by extrapolation from previous values).

More concisely, the following (pseudo-code):

slave.set_input(123);
slave.do_step(1.0);
slave.do_step(1.0);

should be equivalent to this:

slave.set_input(123);
slave.do_step(2.0);

mrindar · 2019-02-26T11:04:33Z

Aah, ok. I thought we agreed on the exact opposite in the previous mob session xD

hplatou

This looks good to me.

kyllingstad · 2019-02-26T12:02:19Z

Aah, ok. I thought we agreed on the exact opposite in the previous mob session xD

Well, any disagreements definitely need to be laid on the table now. I encourage you all to double-check the relevant parts of the FMI (2.0) spec and give me your interpretations of it. I may very well be wrong in this, I don't know it all by heart even if I sometimes pretend to. ;)

PS: I accidentally edited @mrindar's post to write this reply, instead of starting a new comment. Apologies, and reverted now.

mrindar · 2019-02-26T13:49:40Z

From the FMI 2.0 standard:

FMI for Co-Simulation defines interface routines for the communication between the master and all
slaves (subsystems) in a co-simulation environment. The most common master algorithm stops at each
communication point tci the simulation (time integration) of all slaves, collects the outputs y(tci) from all
subsystems, evaluates the subsystem inputs u(tci), distributes these subsystem inputs to the slaves and
continues the (co-)simulation with the next communication step tci → tci+1 = tci+ hc with fixed
communication step size hc. In each slave, an appropriate solver is used to integrate one of the
subsystems for a given communication step tci → tci+1. The most simple co-simulation algorithms
approximate the (unknown) subsystem inputs u(t), (t > tci) by frozen data u(tci) for tci ≤ t < tci+1. FMI for
Co-Simulation supports this classical brute force approach as well as more sophisticated master
algorithms. FMI for Co-Simulation is designed to support a very general class of master algorithms but it
does not define the master algorithm itself.

I interpret this to mean that it is the master algorithms responsibility to ensure 'input correctness'. So if we have the following stepping scenario:

A: |a1|---|a2|---|a3|---|a4|---|a5|
B: |b1|------------------------|b2|
C: |c1|----------|c2|----------|c3|

where A and B have outputs connected to C, the master algorithm has to decide what to set as input values in |c2|. One possibility is to simply use |a3| and |b1|. Another approach is to use |a3| and a linear interpolation of |b1| and |b2|. More complicated interpolation methods could also be used if the slaves provide derivative information.

Thoughts?

Edit:
Sorry for the insane amount of edits. Formatting is hard!

Edit Edit:
Wait wait wait, I'm being a complete dumbo here. You can't interpolate |b1| and |b2|of course!

kyllingstad · 2019-03-01T06:30:12Z

I interpret this to mean that it is the master algorithms responsibility to ensure 'input correctness'.

I don't. As this is a standards document, and hence must be expected to be precise, I don't think we should interpret anything into it that isn't explicitly stated. And I don't see it stated that the master algorithm must provide all inputs, only that it may do so.

So if we have the following stepping scenario:

A: |a1|---|a2|---|a3|---|a4|---|a5|
B: |b1|------------------------|b2|
C: |c1|----------|c2|----------|c3|

where A and B have outputs connected to C, the master algorithm has to decide what to set as input values in |c2|.

Consider the connection from B to C, and assume that B sends derivatives with its outputs. Hey, that is awesome, now C can extrapolate its corresponding input between communication points for improved accuracy. However, if a simplistic master algorithm then goes and "resets" the value of the input at c2 to the b1 value, then that improvement is lost.

mrindar · 2019-03-01T07:14:31Z

I yield ¯\_(ツ)_/¯

ssadjina · 2019-03-02T21:26:59Z

As stated, in the most simple case (0th order interpolation) you would just hold the last input and continue to use it unless an updated one becomes available. I think both of @mrindar and @kyllingstad 's latest comments are correct as far as I can tell.

Just an academic side note (not really important):

Edit Edit:
Wait wait wait, I'm being a complete dumbo here. You can't interpolate |b1| and |b2|of course!

No, they actually could be in an iterative scheme or when first stepping B, and then stepping C (slaves don't have to step in parallel in all cases).

kyllingstad · 2019-03-05T07:43:56Z

I am working both on a test for this as well as a fix for the caching issue, so I'll close this for now. I'll reopen when the fix is ready to merge.

kyllingstad · 2019-03-06T07:08:23Z

I started changing things so that we only set the input variables where we have received an updated value (as opposed to setting all connected inputs at all time steps). But I've hit a snag and would like some feedback:

We now have the ability to set ~~manipulators~~ functions on input variables. These can be used to modify incoming values. However, we also use them in scenarios, where we use the "override" functionality to set values of unconnected input variables. With my planned change, this will no longer work. An unconnected input variable will not receive any values, and an override will therefore no longer have an effect.

I see two solutions:

State that functions on input variables only modify incoming values, and if there are no incoming values, there will be no modified value either. Find some other solution for scenarios.
Create a special case where we always run functions on input variables that have them, even if there are no incoming values.

Any comments or preferences here?

hplatou · 2019-03-06T07:33:06Z

That’s a good question. I think that the ability to override unconnected inputs is required so we need to find some solution for that. In that sense I think I will be voting for the second solution.

kyllingstad · 2019-03-10T22:00:20Z

I believe I have fixed the caching issue now, so I am reopening. This required separating the "get cache" and "set cache" into two different helper classes in slave_simulator, because they now do caching and manipulation differently.

I've also added a test that verifies the functionality.

This ensures that only the variables that have actually been set() will be cached in `slave_simulator` and passed to `async_slave::set_variables()`. Previously, all *exposed* variables would always be in the cache.

This test verifies that issue #181 has been fixed, i.e., that variable values are only transfered between simulators at common synchronisation points.

kyllingstad · 2019-03-11T06:47:39Z

I think that the ability to override unconnected inputs is required so we need to find some solution for that. In that sense I think I will be voting for the second solution.

I've made it so that when a manipulator is set for a variable, it will always be run on the last set value for that variable. If no value has been set yet, it will use the default value.

hplatou

Nice! This looks good to me.

eidekrist · 2019-03-11T09:14:18Z

I've made it so that when a manipulator is set for a variable, it will always be run on the last set value for that variable. If no value has been set yet, it will use the default value.

From reading the code, it seems the manipulator will only run once, and after that only if/when a new value is set with set_value(). So for parameters and unconnected inputs, a manipulator will only run once, which may be a problem for future stateful manipulators such as noise or ramps.

eidekrist

While I like the updated test, I am missing a test case for the original predicament leading to this PR;
A fast slave has an input connected to the output from a slow slave, and we want to verify that the fast slave only gets its input updated at "matching" steps. Right now we only test the opposite case (fast -> slow), but that might be sufficient?

eidekrist · 2019-03-11T09:22:50Z

src/cpp/slave_simulator.cpp

+    // specifies whether they have been run on the values currently in
+    // `values_`.
+    std::unordered_map<variable_index, std::function<T(T)>> manipulators_;
+    bool hasRunManipulators_ = false;


Is hasRunManipulators_ mainly a guard against setting values and manipulators during slave_simulator::set_variables()?

Partly, but that's not its primary purpose. It is used in set_variable_cache::manipulate_and_get() to check whether the manipulators have already been run, to avoid running them multiple times on the same values if the function is called again. (That never happens now, but could easily happen in the future.)

Compare with how it was done before, and still is in get_variable_cache, where you first call run_manipulators() and then access the manipulatedValues vector directly. In the new class, I've made the member variables private and tried to offer a less error prone interface.

ljamt · 2019-03-11T10:02:29Z

From reading the code, it seems the manipulator will only run once, and after that only if/when a new value is set with set_value(). So for parameters and unconnected inputs, a manipulator will only run once, which may be a problem for future stateful manipulators such as noise or ramps.

Can we use the ramp as a case? Will it be sufficient that the "manipulator-function" takes delta-t as an argument?

kyllingstad · 2019-03-11T13:35:46Z

From reading the code, it seems the manipulator will only run once, and after that only if/when a new value is set with set_value().

Correct.

So for parameters and unconnected inputs, a manipulator will only run once, which may be a problem for future stateful manipulators such as noise or ramps.

Dang, you're right. I didn't think of that use case at all. Good catch!

Possible solutions, off the top of my head:

Always update at each time step when a manipulator is present. (Simplest, but feels a bit crude.)
Add a parameter to simulator::set_xxx_manipulator() that allows client code to specify whether the value should be updated at each time step or only when there are incoming values. (Does the client code always know which to choose, though?)
Add some machinery in both algorithm and simulator implementations whereby an algorithm can tell a simulator whether each of its variables is connected. (Overengineered?)

kyllingstad · 2019-03-11T13:38:52Z

While I like the updated test, I am missing a test case for the original predicament leading to this PR;
A fast slave has an input connected to the output from a slow slave, and we want to verify that the fast slave only gets its input updated at "matching" steps.

I agree. I'll add a test.

Right now we only test the opposite case (fast -> slow), but that might be sufficient?

Technically, it's sufficient to test the current implementation because lcm(a, b) == lcm(b, a) for all a, b. But that implementation could change, so a test is definitely in order.

kyllingstad · 2019-03-11T13:42:08Z

Can we use the ramp as a case? Will it be sufficient that the "manipulator-function" takes delta-t as an argument?

The manipulator functions don't get the current time, nor the step size, at the moment, so more changes are needed to support that case.

eidekrist · 2019-03-11T16:20:38Z

Possible solutions, off the top of my head:

Always update at each time step when a manipulator is present. (Simplest, but feels a bit crude.)

Add a parameter to simulator::set_xxx_manipulator() that allows client code to specify whether the value should be updated at each time step or only when there are incoming values. (Does the client code always know which to choose, though?)

Add some machinery in both algorithm and simulator implementations whereby an algorithm can tell a simulator whether each of its variables is connected. (Overengineered?)

This is starting to feel like a design choice. Until we hash out a master plan for the intended usage of these manipulator functions - i.e. how often will they be called, how good of an idea is statefulness etc., and as long as there are no current stateful manipulator function implementations, I'm OK with tackling this in a future issue.

The manipulator functions don't get the current time, nor the step size, at the moment, so more changes are needed to support that case.

Another input to this debate is the cse::manipulator class, which gets notified each time a step commences. This could also be taken advantage of when it comes to time dependent manipulations.

kyllingstad · 2019-03-12T05:16:24Z

While I like the updated test, I am missing a test case for the original predicament leading to this PR;
A fast slave has an input connected to the output from a slow slave, and we want to verify that the fast slave only gets its input updated at "matching" steps.

I agree. I'll add a test.

Done.

eidekrist

Looks good to me! There was a seemingly random test failure on Jenkins which had me worried, so we might have to to keep our eyes open here 😃

kyllingstad · 2019-03-13T06:31:49Z

Looks good to me! There was a seemingly random test failure on Jenkins which had me worried, so we might have to to keep our eyes open here 😃

Hm, that is worrisome indeed. And given that it fails in exactly the same way on two different platforms makes it seem less random. I'll look into it a bit more before merging the PR.

kyllingstad · 2019-03-15T12:35:46Z

Found the problem: #213

It is really unrelated to this PR, so I'll just go ahead and merge now. (It was detected by an assert call in this PR, though, so yay for assert!)

Synchronised variable transfer

c2cba71

This fixes issue #181, "Only transfer variable values at common communication points".

kyllingstad added the bug Something isn't working label Feb 25, 2019

kyllingstad self-assigned this Feb 25, 2019

kyllingstad requested review from ljamt, hplatou, mrindar and eidekrist February 25, 2019 14:33

hplatou approved these changes Feb 26, 2019

View reviewed changes

mrindar approved these changes Feb 26, 2019

View reviewed changes

Get rid of some magic numbers in test

d3bb10c

kyllingstad closed this Mar 5, 2019

kyllingstad reopened this Mar 10, 2019

kyllingstad force-pushed the bugfix/181-only-transfer-values-at-synch-points branch from f134121 to e04e26e Compare March 10, 2019 22:06

kyllingstad added 2 commits March 10, 2019 23:08

Cache values only when the variables are set

4bb42ad

This ensures that only the variables that have actually been set() will be cached in `slave_simulator` and passed to `async_slave::set_variables()`. Previously, all *exposed* variables would always be in the cache.

Test that values are transferred at correct times

9c78d76

This test verifies that issue #181 has been fixed, i.e., that variable values are only transfered between simulators at common synchronisation points.

kyllingstad force-pushed the bugfix/181-only-transfer-values-at-synch-points branch from e04e26e to 9c78d76 Compare March 10, 2019 22:08

kyllingstad requested review from hplatou and mrindar March 10, 2019 22:08

hplatou approved these changes Mar 11, 2019

View reviewed changes

eidekrist reviewed Mar 11, 2019

View reviewed changes

Add test for slow-to-fast-slave connections

4fe4fb5

kyllingstad mentioned this pull request Mar 12, 2019

Support stateful and/or time-dependent manipulators #203

Closed

eidekrist mentioned this pull request Mar 12, 2019

Reset overrides from outside #206

Merged

eidekrist approved these changes Mar 12, 2019

View reviewed changes

kyllingstad merged commit ed49ed4 into master Mar 15, 2019

kyllingstad deleted the bugfix/181-only-transfer-values-at-synch-points branch March 15, 2019 12:35

kyllingstad mentioned this pull request Mar 15, 2019

Only transfer variable values at common communication points #181

Closed

Synchronised variable transfer #189

Synchronised variable transfer #189

Conversation

kyllingstad commented Feb 25, 2019

eidekrist commented Feb 26, 2019

mrindar commented Feb 26, 2019

kyllingstad commented Feb 26, 2019

kyllingstad commented Feb 26, 2019 • edited Loading

mrindar commented Feb 26, 2019

ssadjina commented Feb 26, 2019

mrindar commented Feb 26, 2019

kyllingstad commented Feb 26, 2019 • edited Loading

kyllingstad commented Feb 26, 2019

kyllingstad commented Feb 26, 2019

mrindar commented Feb 26, 2019 • edited by kyllingstad Loading

hplatou left a comment

Choose a reason for hiding this comment

kyllingstad commented Feb 26, 2019

mrindar commented Feb 26, 2019 • edited Loading

kyllingstad commented Mar 1, 2019

mrindar commented Mar 1, 2019 • edited Loading

ssadjina commented Mar 2, 2019 • edited Loading

kyllingstad commented Mar 5, 2019

kyllingstad commented Mar 6, 2019

hplatou commented Mar 6, 2019

kyllingstad commented Mar 10, 2019

kyllingstad commented Mar 11, 2019

hplatou left a comment

Choose a reason for hiding this comment

eidekrist commented Mar 11, 2019

eidekrist left a comment

Choose a reason for hiding this comment

eidekrist Mar 11, 2019

Choose a reason for hiding this comment

kyllingstad Mar 11, 2019 • edited Loading

Choose a reason for hiding this comment

ljamt commented Mar 11, 2019

kyllingstad commented Mar 11, 2019

kyllingstad commented Mar 11, 2019

kyllingstad commented Mar 11, 2019

eidekrist commented Mar 11, 2019

kyllingstad commented Mar 12, 2019

eidekrist left a comment

Choose a reason for hiding this comment

kyllingstad commented Mar 13, 2019

kyllingstad commented Mar 15, 2019

kyllingstad commented Feb 26, 2019 •

edited

Loading

kyllingstad commented Feb 26, 2019 •

edited

Loading

mrindar commented Feb 26, 2019 •

edited by kyllingstad

Loading

mrindar commented Feb 26, 2019 •

edited

Loading

mrindar commented Mar 1, 2019 •

edited

Loading

ssadjina commented Mar 2, 2019 •

edited

Loading

kyllingstad Mar 11, 2019 •

edited

Loading