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

Improve supporting for XMILE specification #144

Merged

Conversation

alexprey
Copy link
Collaborator

I'm check the XMILE specification and try to mention the possible issues that should to implement.
I not mention in this file problem with parsing the IF ... THEN ... ELSE statement, but it should be also supported by grammar. In nearest future I want to continue improve specification supporting.

The first I want to fix small, but where important issues:

  1. IF .. THEN .. ELSE
  2. Inline comments
  3. PULSE function

Regards,
Alexey from sdCloud.io development team.

@coveralls
Copy link

coveralls commented Sep 20, 2017

Coverage Status

Coverage decreased (-7.9%) to 80.8% when pulling d6ee6ee on alexprey:structuring_readers into 6c7972b on JamesPHoughton:structuring_readers.

@coveralls
Copy link

coveralls commented Sep 21, 2017

Coverage Status

Coverage decreased (-85.6%) to 3.116% when pulling b571a9a on alexprey:structuring_readers into 6c7972b on JamesPHoughton:structuring_readers.

@coveralls
Copy link

coveralls commented Sep 21, 2017

Coverage Status

Coverage decreased (-0.8%) to 87.919% when pulling b18e118 on alexprey:structuring_readers into 6c7972b on JamesPHoughton:structuring_readers.

@alexprey alexprey changed the title Reformat file and compare definitions with XMILE specification Improve supporting for XMILE specification Sep 21, 2017
@coveralls
Copy link

coveralls commented Sep 21, 2017

Coverage Status

Coverage decreased (-8.6%) to 80.04% when pulling 6f72dd2 on alexprey:structuring_readers into 6c7972b on JamesPHoughton:structuring_readers.

@coveralls
Copy link

coveralls commented Sep 21, 2017

Coverage Status

Coverage decreased (-0.8%) to 87.905% when pulling 0aaed42 on alexprey:structuring_readers into 6c7972b on JamesPHoughton:structuring_readers.

@coveralls
Copy link

coveralls commented Sep 21, 2017

Coverage Status

Coverage decreased (-0.7%) to 87.939% when pulling f0f2fed on alexprey:structuring_readers into 6c7972b on JamesPHoughton:structuring_readers.

@alexprey
Copy link
Collaborator Author

@JamesPHoughton need small help from you. I can't understand how works the imports in python. On my local setup import on this line from .py_backend import functions is not work with out this line from . import py_backend in pysd\__init__.py. But on travis this is not work.

@alexprey
Copy link
Collaborator Author

@JamesPHoughton also I want to say that xmile parser works fine, but model execution is not works, because it is depends on specific definitions in vensim models. F.ex. after run I have the next error File "pysd\py_backend\functions.py", line 555, in _format_return_timestamps self.components.final_time() + self.components.saveper(),. Can you look on this?

@JamesPHoughton
Copy link
Collaborator

@alexprey - I haven't figured out how to do imports on this package in a way that works for both python2 and python3. Its a problem. Right now, should be working with py3 - I see on travis that that build at least runs most of the tests.

The last issue is because of the saveper. Right now, the code manually adds the components vensim uses for initial and final times here, but the saveper is missing. We should put it in for now, although I don't know if this is a solution I want to stick with in the long term. You probably agree. Its not elegant.

@alexprey
Copy link
Collaborator Author

@JamesPHoughton can I try to change logic with it and use something abstract for computation steps?

@JamesPHoughton
Copy link
Collaborator

Hey @alexprey, if you want to merge my recent changes into your branch, i'll complete this PR and we'll be on the same page. Otherwise, I'm happy to pull pieces into my branch.

…on/pysd into structuring_readers

# Conflicts:
#	pysd/py_backend/functions.py
@alexprey
Copy link
Collaborator Author

@JamesPHoughton of course, I'll merge your branch and solve conflicts.

@alexprey
Copy link
Collaborator Author

@JamesPHoughton can you check requirements.txt file? Travis-CI can't download one of required dependency...

@JamesPHoughton
Copy link
Collaborator

JamesPHoughton commented Sep 28, 2017

Good call, looks like scipy is an issue for others as well:
travis-ci/travis-ci#2650 (comment)

  • looks like the tests are at least running for py3. py2 has some weird issues I'll need to debug.

@coveralls
Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage decreased (-86.8%) to 3.922% when pulling d39de32 on alexprey:structuring_readers into 1b343cf on JamesPHoughton:structuring_readers.

@coveralls
Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage decreased (-0.8%) to 89.991% when pulling 3cf1765 on alexprey:structuring_readers into 1b343cf on JamesPHoughton:structuring_readers.

# http://docs.oasis-open.org/xmile/xmile/v1.0/csprd01/xmile-v1.0-csprd01.html#_Toc398039982
# ====

# "delay" !TODO!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delay functions create additional structure in the model, so they'll eventually be part of a dictionary called builders. Then what happens is when the parser encounters a delay call, it'll run the builder function that makes delays, using the appropriate arguments, add the new structure to the list of model elements, and then substitute into the equation a reference to that new structure. As an example, in the translation from vensim, the INTEG call triggers the parser to call the builder.add_stock() function, which adds this line as new structure, and inserts a call to that object in the original function. It may not be the best way to do it, but it makes it more straightforward to keep track of documentation and function names, without adding too much complexity.

I've created an empty dictionary in this file builders = {} just as a placeholder. Lets put these delay and smooth and trend functions in there.

# http://docs.oasis-open.org/xmile/xmile/v1.0/csprd01/xmile-v1.0-csprd01.html#_Toc398039980
# ===

"abs": "abs",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the way you've cleaned this up. Much more readable. Thanks!

# ===

# !TODO!
# This function works with wrong way for XMILE specification: http://docs.oasis-open.org/xmile/xmile/v1.0/csprd01/xmile-v1.0-csprd01.html#_Toc398039983
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that vensim's pulse behaves differently from XMILE's? If that's the case, we should have both in the functions file and just make sure we call the right one in the translator.

# ===
# 3.5.6 Miscellaneous Functions
# http://docs.oasis-open.org/xmile/xmile/v1.0/csprd01/xmile-v1.0-csprd01.html#_Toc398039985
# ===
"if_then_else": "functions.if_then_else",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may be able to get away with replacing if/then/else individually with their python safe equivalents. If not, we'll have to add a special line to the grammar to handle and translate them. I agree, lets get the other bits working and come back to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean using the native python if then else construction instead the buildin function?

@@ -79,6 +168,8 @@ def parse(self, text, context='eqn'):
If context is set to equation, lone identifiers will be parsed as calls to elements
If context is set to definition, lone identifiers will be cleaned and returned.
"""
# !TODO! Should remove the inline comments from `text` before parsing the grammar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline comments - good point. I have a hunch that it would be cleanest to pull these out using the grammar and parser, as opposed to preprocessing with a regex, but open to trying it each way.

Is there a sample model that includes comments? If not, we'll need to make one and add it to the test-models repo before tackling this.

@JamesPHoughton JamesPHoughton merged commit 91ce868 into SDXorg:structuring_readers Sep 29, 2017
@JamesPHoughton
Copy link
Collaborator

@alexprey

Thanks for all your work on this! I really appreciate the collaboration. =)

If you've got the appetite for it, would you mind adding a test of the XMILE syntax pulse/ramp etc. input functions to the test repository? It would be good to have a test for your pulse_magnitude function... I have similar tests for vensim here - so you could either translate this vensim model into xmile (checking that it still produces the same output, or create a new directory with input function test specific to xmile.

@alexprey
Copy link
Collaborator Author

alexprey commented Sep 29, 2017 via email

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

Successfully merging this pull request may close these issues.

3 participants