feat: integrate evaluated var in transform/expression.py/expression() #93
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
+ Coverage 68.73% 69.17% +0.44%
==========================================
Files 37 37
Lines 3272 3277 +5
==========================================
+ Hits 2249 2267 +18
+ Misses 1023 1010 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| def test_transform(name, kwargs, out, value): | ||
| transform = ALL_TRANSFORMS[name](**kwargs) | ||
| ds = transform(DATA[name]) | ||
| print(ds[out].mean().values) # this value should be the correct one |
There was a problem hiding this comment.
This is helpful to figure out what the value is to check against, but then should be removed in the commit.
| expr : str | ||
| An algebraic expression of the form "a = b + c" defining how to compute a new variable. | ||
| The lhs is the output variable name, and the rhs is a valid Python expression referencing existing dataset variables. | ||
| integrate_time : bool, optional |
There was a problem hiding this comment.
The only thing that I am not sure about is the name of this parameter. You know that a mean is not a lot different than a integral and can see that maybe sometimes we would want to integrate it. Since we talked about this I came to a realization:
By adding in this time mean to expressions, we are now making the transform do 2 things. I wonder if we should rather make a integrate_time transform with an option for mean or not which could be called in sequence before the expression transform.
This is the messy part of design. I cannot tell you how many times I have implemented something only to re-implement it not long after. I think, we keep this as is and let's chat about the above idea and see if we can agree that makes more sense :D
Closes #57
This allows the user to specify if they want to integrate the input variables of the expression over time before evaluation. This is a small addition to
expression()that prevents us from writing unnecessary transforms that only require simple algebra.Thought: This may need to be adjusted so that the user can specify mean or total. Right now, it is locked on mean.