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

Corrected active_initial (from Vensim) behavior #184

Open
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ainar
Contributor

ainar commented Jun 20, 2018

This function didn't prevent circular init (with my World3 again...)

I passed the parameters with "lambda" expressions to avoid premature evaluation of the first parameter (the thing that we exactly do not want by calling active_initial) :

  • I adjusted the parser to correctly create the calls of functions.active_initial
  • I edited functions.active_initial to only evaluate expression if the initialization step is over

With that update, World3 Vensim model is (normally) fully transpilable and runnable with PySD, without any manual edit (I invite you to try), but it may break compatibility with py models that use former active_initial implementation.

I add a cast to np.float64 before rewriting the numeric constants into the Python model because I noticed that the type of the number changes the output of my model. Particularly the variable perceived_food_ratio of W3 stay at 1 the two first steps and start to increase only at step 3 if I let some init at 1, but start to increase at the second step (as in Vensim) if we put 1.0... I am afraid that this int/float difference has other impacts.

Do I need to create a unit test and a test model for this too?

@coveralls

This comment has been minimized.

coveralls commented Jun 20, 2018

Coverage Status

Coverage increased (+0.2%) to 89.614% when pulling db20a63 on ainar:master into e348dba on JamesPHoughton:master.

@alexprey

This comment has been minimized.

Contributor

alexprey commented Jun 20, 2018

Looks like that everything was broken....

@ainar

This comment has been minimized.

Contributor

ainar commented Jun 20, 2018

Why did it run only two jobs on coveralls instead of 3? :s
I don't really understand...

steep
@ainar

This comment has been minimized.

Contributor

ainar commented Jun 20, 2018

Yeah! I increased the coverage rate! Just a last commit to resolve the conflicts

steep
def visit_active_initial(self, n, vc):
string = "functions.active_initial(lambda: %(expr)s, %(init)s)" % {
'expr': vc[4],
'init': str(np.float64(vc[8]))

This comment has been minimized.

@alexprey

alexprey Jun 20, 2018

Contributor

What if the second argument of active_initial may be is expression? So this conversion will fail. Or vensim specification allow only constants? I'm not spec of vensim :)

@alexprey

This comment has been minimized.

Contributor

alexprey commented Jun 20, 2018

Can you provide simple example model with same error? Maybe we can found another way to solve issue

@ainar

This comment has been minimized.

Contributor

ainar commented Jun 21, 2018

The ACTIVE INITIAL example provided by Vensim is even not working with PySD...
https://vensim.com/documentation/Models/FunctionExamples/ACTIVE%20INITIAL.mdl

Here is the doc : https://vensim.com/documentation/index.html?fn_active_initial.htm
I don't know if it requires a constant but it seemed legit. It needs some tests on Vensim to check that.

@ainar

This comment has been minimized.

Contributor

ainar commented Jun 21, 2018

Ok it works for me...

Sorry for the mess T-T

@alexprey

Hi @ainar, I'm also look at example and implementation, so that this way of fix is OK for now. Just make a small fixes and I think that @JamesPHoughton can merge it 👍

@@ -375,7 +376,7 @@ def parse_units(units_str):
"ramp": "functions.ramp",
"min": "np.minimum",
"max": "np.maximum",
"active initial": "functions.active_initial",
#"active initial": "functions.active_initial",

This comment has been minimized.

@alexprey

alexprey Jun 21, 2018

Contributor

Please remove this line instead commeting

call = func _ "(" _ (expr _ ","? _)* ")" # these don't need their args parsed...
active_initial = ~r"active initial"I _ "(" _ expr _ "," _ expr _ ")"

This comment has been minimized.

@alexprey

alexprey Jun 21, 2018

Contributor

Rename this statement to active_initial_call for consistency

@alexprey

This comment has been minimized.

Contributor

alexprey commented Jun 21, 2018

Thanks @ainar, this is an interested case and it's a good time for founding that :)
At now I'm work at refactoring of function builders for xmile and vensim, and after refactoring this issue can be solved by correct function definition. You can see changes in this commit if you interested: alexprey@df14160


Regards,
Alexey from sdCloud.io development team.

steep added some commits Jun 22, 2018

@ainar

I prevent the fact that we want to evaluate the init val ONLY on initialization stage (and not after)

@ainar ainar force-pushed the ainar:master branch from 324c76f to db20a63 Jun 26, 2018

@JamesPHoughton

This comment has been minimized.

Owner

JamesPHoughton commented Jun 26, 2018

Hi @alexprey and @ainar - just having a look at this! Thanks for making so much progress. =)

If I understand correctly, the unit test we had for ACTIVE INITIAL had been passing, but there was still a problem with active initial because:

  1. the function to find the initialization value runs at other times in the simulation. This doesn't cause circular reference, it just is unnecessary computation.
  2. the function to find the runtime value is also called at initialization, and can cause a circular reference.

Is that correct? If so, we probably should update the test case in the test models repository so that it fails when it should. @ainar is that something you'd mind doing?

To fix the problem, we need the parameters to functions.active_initial to be functions, not values, so that we can choose which to evaluate. (Or we could use some sort of short-circuiting logic in place of an active_initial function, but that could get messy.)

Another way to do this could be to add to the builders dictionary:

"active initial": lambda element, subscript_dict, args: (
    "functions.active_initial(lambda: %s, lambda: %s)" % args, 
    []
)

Then we wouldn't have to add anything to the grammar. What do you think? Is this less clear?

It is possible that there are other situations in which we'll need delayed evaluation. If there are lots, we might consider adding another whole category (like 'builders') that handles functions with delayed evaluation.

@JamesPHoughton

This comment has been minimized.

Owner

JamesPHoughton commented Jun 26, 2018

Not to second-guess the way you've done it - just a bias towards parsimony. =)

@alexprey

This comment has been minimized.

Contributor

alexprey commented Jun 26, 2018

@JamesPHoughton yep, you are right. We need to add additional test for circular reference case.

It is possible that there are other situations in which we'll need delayed evaluation. If there are lots, we might consider adding another whole category (like 'builders') that handles functions with delayed evaluation.

I'm work for that 👷 alexprey@df14160

@ainar ainar force-pushed the ainar:master branch from 7349d66 to db20a63 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment