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.

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

Accessing model variables from macro with trailing $ syntax #143

Open
Heldera opened this issue Sep 9, 2017 · 5 comments
Open

Accessing model variables from macro with trailing $ syntax #143

Heldera opened this issue Sep 9, 2017 · 5 comments

Comments

@Heldera
Copy link

Heldera commented Sep 9, 2017

I have a model
integration3.mdl . When I try to run it I got following error:
Parse error at line "mean+white noise*std deviation*SQRT((2-time step$/correlation time)/(time step$/correlation time))", position: 30


Regards,
Olga from sdCloud.io

@JamesPHoughton
Copy link
Collaborator

Hi Olga,

There seem to be two things going on here that I don't understand, both related to that $ in time step$.

  1. One is that model elements with special characters usually have double quotes, so I would have expected to see "time step$". It may be that dollar signs are somehow exempt from that rule, we'll have to look it up. If so, we'll need to correct the parser to be able to capture names with dollar signs as standard identifiers, outside of double quotes.

  2. The second is the keyword itself, time step$, which I assume means something specific, as it isn't just time step on its own, and it isn't defined elsewhere in the macro. Also, the note in the comment about Tom adding the TIME STEP$ keyword suggests that this means something interesting.

Do you have any insight into either of these?
James

@JamesPHoughton
Copy link
Collaborator

Appears that the $ sign allows a macro to access the (calling) model's namespace:

The one exception to the local nature of macro variables is that you may specify a model variable by following its name with a dollar sign $. For example you can use TIME STEP$ inside of a macro to refer to the model variable TIME STEP. Only unsubscripted variables may be referred to in this manner. (From the Vensim Help)

Because the macro parser is just the same as the model parser, we'll have to modify this to handle the differing model namespaces. The model may need to have some awareness of the fact that the macro 'reaches out' of its namespace, and may actually be responsible for inserting variables into that macro. This really breaks the paradigm of the macro as a function that can be called from the main body of the model. Why shouldn't these variables just be passed into the macro? The best way to do this in PySD (if we choose to support this) is to essentially rewrite the macro to include the $'d variable in the parameter list. Either that or just depend on python's namespacing rules to look for the variable in the outer scope.

Not sure if this would work. This type of thing would be ok:

def outer():
    i = 3
    def inner():
        print(i)
    inner()
    
outer()

3

But I think the way the macro is instantiated, it would be a bit more like this:

def inner():
    return i + 1

def outer():
    i = 3
    return inner() + 1
    
outer()

error

@Heldera - do you have a sense for how ubiquitous this pattern is? Is it something that should really be supported, or can we get away with ignoring?

@JamesPHoughton
Copy link
Collaborator

JamesPHoughton commented Sep 12, 2017

Anyways, I suppose we need a test model either way, just to be complete about things. @Heldera , would you mind putting one together that includes this syntactic abomination, for the test repo?

-- very grateful.

@JamesPHoughton JamesPHoughton changed the title Error on vensim model parsing Accessing model variables from macro with trailing $ syntax Sep 12, 2017
@enekomartinmartinez
Copy link
Collaborator

Hi @Heldera,
Thank you for opening the issue. We have made several improvements in PySD since you opened it and some things may have changed. If you still are interested in using PySD I recommend you to updtate it to the last version.

I think this bug should be corrected. If you still are experiencing it let me know.

@enekomartinmartinez
Copy link
Collaborator

Sorry @Heldera,
I have been checking the model file and this has not been implemented yet. Ignore my last message.

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

No branches or pull requests

3 participants