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

Fixes unittest fail due to PyYAML upgrade #46

Merged
merged 9 commits into from Apr 25, 2020
Merged

Conversation

tclose
Copy link
Contributor

@tclose tclose commented Apr 12, 2020

No description provided.

@tclose
Copy link
Contributor Author

tclose commented Apr 12, 2020

New failures due to upgrade in Sympy's evalf. Currently investigating how to work around them

@tclose
Copy link
Contributor Author

tclose commented Apr 12, 2020

@sanjayankur31 Will hopefully have them sorted out soon

@tclose
Copy link
Contributor Author

tclose commented Apr 14, 2020

I have fixed errors that were causing the tests to fail for Python 3.5-3.7. However, 2.7 is failing due to a function within Sympy not working with callable objects (instead of a function) when using PY2.

Looks like it would be quite a bit of work to come up with a work around, which to my mind doesn't seem worth the trouble now that Python 2 is end-of-life. @apdavison, do you have any thoughts about dropping Python 2 support? Do you think it is worth the hassle at this stage?

@apdavison
Copy link
Member

Let's drop it.

@tclose
Copy link
Contributor Author

tclose commented Apr 14, 2020

Working around the PY2 issue turned out to be pretty straightforward in the end (came to me straight afterwards) so now all builds pass including Python 2.7. So we can keep it in until something else comes up.

@tclose tclose requested a review from apdavison April 14, 2020 11:41
@tclose
Copy link
Contributor Author

tclose commented Apr 14, 2020

Note that I have added numpy to the list of requirements to handle array values

.format(self.rhs_str))
return val
return nineml_expression
return lambdify(list(self.rhs_symbol_names), self.rhs, 'numpy')
Copy link
Member

Choose a reason for hiding this comment

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

does this single line really achieve the same as the deleted code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and runs much faster for complex expressions as it uses Numpy under the hood :).

I think I must have missed lambdify when I was converting the library to Sympy, either that or it was added later. I have been using it in my ref_implementation branch so I am fairly familiar with how it works now. Also, if you have a look at that code it mostly just uses the evalf method but then has to handle the special cases when the expression is a relational (i.e. in Triggers) or has a built-in function in it (the eval at the bottom). Using Numpy avoids having to do all these things.

I had also been avoiding Numpy as a dependency as I liked the idea that the library was pure Python and then could be run in other languages (e.g. Java, Iron Python). But that is probably a very marginal gain and not worth the ease of handling array data with Numpy.

@apdavison apdavison merged commit 062a2ac into INCF:master Apr 25, 2020
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.

None yet

2 participants