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

New functions #1

Open
wants to merge 4 commits into
base: new_functions
Choose a base branch
from

Conversation

DVRodri8
Copy link

This changes add support for more complex subscripts and excel operations. I have created the folder my_tests on tests folder with one test, this should be included on the repository SDXorg, but i've added there to show how it increases the coverage up to 87%. So, it is closer to the 91% of PySD master, maybe this branch can be merged with it.

@julienmalard
Copy link
Owner

Thank you! I have been working on other projects lately and so will need to take a bit of time to refresh my mind. I do not see the test results, but if they pass, I think this would be a good addition. So the workflow would be to merge to this branch, then merge this branch to the main PySD repository?

@DVRodri8
Copy link
Author

You can execute the test by yourself executing "nosetests --with-coverage --cover-package=pysd --cover-erase" inside the tests directory.

If that results are good enought to PySD owners we should include the Vensim example that now is on the directory "my_tests" into the SDXorg repository in order to remove "my_tests" directory (it's a quick hack to show you how we can improve test coverage). And then i think we can merge this branch with the main PySD repository.

@enekomartinmartinez
Copy link

enekomartinmartinez commented Sep 17, 2020

Hi,
First I want to thank you both for your contribution.
I have some issues about the last version that maybe can be improved before doing the merge to the master branch.

The data read from the excel is stored in cache as 'step' data, I think it would make more sense to store it as 'run' as the excel input data is not supposed to change. I have been trying to correct that but I am a little bit messed up about how the parser works, so maybe for you is easier.

Moreover, when working with both float values and values coming from dataarrays some functions may fail. I have fixed that, I will add an example with a test.

@enekomartinmartinez
Copy link

Hi all!
I have almost finished my development for these features. I have made several big changes and added new possibilities to the Excel reading such as using cellrange names (only available for python versions where openpyxl is available). At this point, I have a global coverage of 95%, but I expect it to be a little bit bigger as I still have some extra tests to add, in any case, it is already bigger than the current coverage of PySD.
I have corrected some bug, done some improvements for reliability and speed, documentated all the new functions and added several tests, most of them to check the reading of DATA/CONSTANT/LOOKUPS from files in several ways, so we can ensure that it will work fine always.

I wanted to ask you how do you want to proceed to do a new pull request to the main PySD repository. Do you want me to do a pull request to your repositories or do you prefer me todo a pull request directly to the main repository? In the second case I would cite this pull request and mention your work.

@julienmalard
Copy link
Owner

Thank you @enekomartinmartinez ! Sure, if you want to make a pull request here we can merge here and then in the main repository (whichever is easiest for you).

@enekomartinmartinez
Copy link

So I suppose I will make a direct pull request to the main repository, right now I'm waiting to accept two pull requests in the models' repository. As soon as they accept them I will make the pysd pull request, as I need to update the models' repo with the new commit.

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