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

Solves & Fixes Issue #70 | plot() doesn't produce an error now #72

Closed
wants to merge 5 commits into from
Closed

Solves & Fixes Issue #70 | plot() doesn't produce an error now #72

wants to merge 5 commits into from

Conversation

ashutoshsaboo
Copy link
Contributor

@asmeurer .

This PR solves and fixes issue #70 . plot() no longer gives an error. The main error was because of the numpy and django version on my system, my local installation of numpy and django were very new version's in comparison to those compatible with SymPy Gamma, as of now. Re-installing the required versions of numpy and django solved the issue! Hence, I modified the test case file, as well as the requirements file. Now, plot() card in my local installation of SymPy Gamma doesn't produce any error, and displays the required graph.

Hence, this fixes & resolves issue #70 .

The same can be observed in this screenshot-:

selection_009

@ashutoshsaboo
Copy link
Contributor Author

@asmeurer So, this has passed all the tests now. I guess it'll be better if you merge this PR - #72 first, and then you can merge PR - #69 , as both of them have passed all tests now.

@asmeurer
Copy link
Member

I think the only change here should be the requirements.txt file. You have mixed in changes from other pull requests. Make sure when you start a new branch that you git checkout master first so that your branch is based off of master.

@ashutoshsaboo
Copy link
Contributor Author

@asmeurer Ohh! So, what should I do now? I guess it'd be better for me to handle this once PR #69 is merged, because I guess it's files have only come here. Or is it fine like this?

@asmeurer
Copy link
Member

Could you create a new pull request with a new branch, which is based off of master, with just the change to the requirements file. If the fix really is correct, the phantom.js change should not be necessary.

@ashutoshsaboo
Copy link
Contributor Author

@asmeurer Actually as I said, the phatom.js change is required for the description I provided here (second last comment) #69 . That's the thing.

@asmeurer
Copy link
Member

If the phantom.js change is needed then you haven't really fixed the issue, because all the phantom.js change does is disable the test. I am -1 to the phantom.js change.

@ashutoshsaboo
Copy link
Contributor Author

Ohh @asmeurer . I'll try pushing a PR with only the requirements.txt change.

@ashutoshsaboo
Copy link
Contributor Author

The requirements.txt fix at least fixed everything up on my local system. I have pushed a new PR. Now, let's see what happens.

@ashutoshsaboo
Copy link
Contributor Author

So, I'll close this PR, since #70 is merged.

@ashutoshsaboo ashutoshsaboo deleted the changes1 branch March 18, 2016 17:15
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.

2 participants