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

Implementing reader-side Get()s for ExpressionString derived variables #4249

Merged
merged 7 commits into from
Aug 21, 2024

Conversation

eisenhauer
Copy link
Member

No description provided.

@eisenhauer eisenhauer force-pushed the ExprStrReader branch 2 times, most recently from 04bac7d to 02e1248 Compare August 14, 2024 18:21
@eisenhauer eisenhauer changed the title Non-functional approach to ExpressionString derived variables Implementing reader-side Get()s for ExpressionString derived variables Aug 14, 2024
@eisenhauer
Copy link
Member Author

@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.

@anagainaru
Copy link
Contributor

Will look at this tonight, it needs more attention than other PRs

@eisenhauer
Copy link
Member Author

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.)

Copy link
Contributor

@anagainaru anagainaru left a 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.

source/adios2/toolkit/derived/Expression.cpp Outdated Show resolved Hide resolved
anagainaru
anagainaru previously approved these changes Aug 21, 2024
Copy link
Contributor

@anagainaru anagainaru left a 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.

@eisenhauer
Copy link
Member Author

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?

@eisenhauer eisenhauer merged commit 3bafd27 into ornladios:master Aug 21, 2024
38 checks passed
@eisenhauer eisenhauer deleted the ExprStrReader branch August 21, 2024 19:24
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