-
Notifications
You must be signed in to change notification settings - Fork 39
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
Redo #160 #170
Redo #160 #170
Conversation
bw2io/extractors/simapro_csv.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ccomb this is not your fault but can I ask you to fix something while we're working on this part? Using Python's native eval is unsave for arbitrary user inputs. Could you change the line to use bw2parameter's DefaultInterpreter
instead (or asteval.Interpreter
if Chris does not want to make bw2parameters a mandatory dependency)? That would be very helpful :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exxxcccellent idea to remove an eval. I'll look at it this week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure asteval
is a good choice? I'll read the doc to see if the parser can be configured
>>> asteval.Interpreter().eval("2+2")
4
>>> asteval.Interpreter().eval("2^2")
0
>>> asteval.Interpreter().eval("2**2")
4
>>> asteval.Interpreter().eval("2^-2")
-4
>>> asteval.Interpreter().eval("2**-2")
0
>>> asteval.Interpreter().eval("2.0^-2")
>>> asteval.Interpreter().eval("2.0**-2")
0.25
>>> asteval.Interpreter().eval("2.0^(-2)")
>>> asteval.Interpreter().eval("2.0**(-2)")
0.25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we still convert ^
to **
in the input data, the only problem is the evaluation of 2**-2
. I've asked as an asteval
issue here: https://github.com/newville/asteval/issues/115
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to merge, with eval
replaced with asteval
.
But we need a fixed version of asteval
@@ -15,6 +15,7 @@ | |||
import re | |||
import numpy as np | |||
from stats_arrays import LognormalUncertainty | |||
import bw2parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means we have to add bw2parameters to the setup requirements. What do you say @cmutel?
Hi @ccomb, Let's move the eval replacement to a new PR and get everything else merged for now. Can you roll back the eval change and make sure all tests are passing? Cheers. |
The problem with using only asteval is that the parameter variables are not evaluated and I get a lot of warnings in the console during an ecoinvent import, but I had some troubles with bw2parameter also. I'll try to find some time in the next two or three weeks to find a solution. In the meantime I'll split this PR into smaller ones. |
I created another PR #201 with just asteval |
Fix #156 #159 and redo broken PR #160
Don't pin numpy, evaluate expressions in allocations, convert exponent in simapro csv import