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

Exposes IsReverseDims in BlockInfo #1573

Merged
merged 1 commit into from
Jun 29, 2019
Merged

Conversation

williamfgc
Copy link
Contributor

@williamfgc williamfgc commented Jun 29, 2019

related to request in #1553 from @berkgeveci

Copy link
Contributor

@germasch germasch left a comment

Choose a reason for hiding this comment

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

I guess a general comment is that since IsReverseDims will always be the same across all blocks in a Variable, I'd prefer to have it exposed on a per-Variable basis, not per-Block. Most readers / viz apps will probably not naturally use the block-based API.

@@ -348,6 +348,8 @@ class Variable
size_t Step = 0;
/** reference to internal block data (used by inline Engine) */
const T *Data() const;
/** true: Dims were swapped from column-major, false: not swapped */
Copy link
Contributor

Choose a reason for hiding this comment

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

minor comment: dims may also have been swapped from row-major to col-major, when reading from Fortran.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@germasch this particular struct is only exposed in the C++11 bindings

@williamfgc
Copy link
Contributor Author

@germasch see my comment in #1553, it's not unreasonable to think we might need this info on a per block basis. The block inspection functions in C++ were designed with in-situ viz as one of the use-cases. It's very much needed.

Copy link
Contributor

@chuckatkins chuckatkins left a comment

Choose a reason for hiding this comment

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

See #1553 (comment)

I don't want to push this in the release. We need some time to get it right and work with the apps that need it (both vis and simulations). It's a new feature with only a day left to the release. Let's go ahead and put this in master instead for now so it will be there but still give us time to refine it for the next release.

@chuckatkins
Copy link
Contributor

@williamfgc please edit the PR to target master instead. If we want to put out a patch release soon after with this then we can. For now though, given the short time frame, let's keep it out of the release.

@chuckatkins chuckatkins dismissed their stale review June 29, 2019 12:36

Moving to master

@chuckatkins chuckatkins changed the base branch from release to master June 29, 2019 12:36
@williamfgc
Copy link
Contributor Author

williamfgc commented Jun 29, 2019

@chuckatkins fine with me, just doing this for you and @berkgeveci

@williamfgc williamfgc merged commit 69e5299 into ornladios:master Jun 29, 2019
@williamfgc williamfgc deleted the swap branch June 29, 2019 13:16
@williamfgc
Copy link
Contributor Author

request in #1553

@germasch germasch mentioned this pull request Aug 5, 2019
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.

3 participants