-
Notifications
You must be signed in to change notification settings - Fork 125
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
Implementing reader-side Get()s for ExpressionString derived variables #4249
Conversation
04bac7d
to
02e1248
Compare
@lizdulac and @anagainaru This is a revival of the older PR. Main changes on the writer side are: 1) Like StatsOnly, it adds block-level metadata on the writer side and 2) Unlike StatsOnly, it doesn't actually do any of the allocations or calculations, just goes through the motion so that we get the same blocks info that you'd have generated if you did those derived variable calculations. On the reader side, we do the same stuff that we do for StatsOnly. That is, if someone asks us for a block of a derived variable that we don't have data for, we calculate the data and return it. |
Will look at this tonight, it needs more attention than other PRs |
As of the most recent build, this was run with DerivedVariables enabled, so it does compile and run on all platforms. But master now has that turned off, so once updated or merged it'll be optional, not compiled or tested in CI. (Not sure what's going on with that one failure, but it isn't related to his PR.) |
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.
I would not propagate DoCompute all the way to the math functions. We can stop at the expression level, creating empty DerivedData (as I am suggesting above).
However, the more I think about this, the ExpressionOnly
flag is on the entire Variable so I am not sure why you need to call ApplyExpression at all.
02e1248
to
befc029
Compare
eb6b761
to
98e92bf
Compare
80e1b59
to
7950cdb
Compare
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.
The tests pass with DerivedVariables ON. Greg feel free to revert if you don't like my changes. Otherwise this is ready to merge.
Sorry, had to add one more thing to make sure that bpls isn't looking for min/max that doesn't exist for these vars. Approve again? |
No description provided.