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

Addresses https://github.com/JamesPHoughton/pysd/issues/80 #95

Closed
wants to merge 2 commits into from

Conversation

JoonaTuovinen
Copy link
Contributor

Added warning to set_components when used with stocks
(just warning because it is actually an easy way to analyse models by overriding stock values to constants)
Added test_set_components_warnings
Moved the test_py_model_file and test_mdl_file under class TestPySD

Added warning to set_components when used with stocks
(just warning because it is actually a good way to analyse models by overriding stock values to constants)
Added test_set_component_warnings
Moved the test_py_model_file and test_mdl_file under class TestPySD
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 94.318% when pulling ca854e6 on JoonaTuovinen:master into 2b05895 on JamesPHoughton:master.

@JamesPHoughton
Copy link
Collaborator

This looks great - I think the travis build error is due to the way the model file reference is generated here: https://github.com/JoonaTuovinen/pysd/blob/2421af710744e19501d4d3cfee718ebcd667ae0b/pysd/pysd.py#L125

I'm guessing that the test used the .pyc file instead of .py, and the replace function left the trailing 'c'. I suppose a more robust way to do this would be to have the parser save the original filename directly. Not sure what that would require though...

@JamesPHoughton
Copy link
Collaborator

@JoonaTuovinen would you mind also having a look at this: #79 - its closely related to what you're doing here, and would be good to patch together.

I think you can leave the PR open, and then when you make changes to your copy, it'll update the PR with them appropriately...

@JoonaTuovinen
Copy link
Contributor Author

@JamesPHoughton Yes you're right, more robust for getting the mdl_file would be using the original mdl filename directly. Should I just save it inside the generated .py file?

I think it still makes sense to get the py file by str(self.components.file).replace('.pyc', '.py'). Seems over engineering to save the py file name.

I'll do the #79 too.

I wonder why, when I run the test it doesn't use the compiled file and the test passes.

@JamesPHoughton
Copy link
Collaborator

Actually, do we need the original model file name as a variable? The .py filename is helpful because you may want to load the translated version and skip the work of translating a second time, and so its helpful to have programmatic access to that name.

We might be ok for the original file name just listing it in a docstring at the top of the file. Then if someone opens the translated file they know where it came from.

@JamesPHoughton JamesPHoughton mentioned this pull request Sep 16, 2016
@JamesPHoughton
Copy link
Collaborator

Updated this PR to reflect changes in stateful representation of stocks, etc in #101

@JoonaTuovinen does this work for you?

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