-
Notifications
You must be signed in to change notification settings - Fork 128
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
Restructure to allow direct specification of array order #2763
Conversation
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.
Looks good to me as long as it passes all the tests. For cross-language tests, I think we are good for now. At least I have been often blocked by the Fortran tests when I wanted to merge something, which is saying the Fortran tests are pretty effective. I don't mind if you want to add new tests though :)
This PR lacks: Python bindings, and tests of its actual functionality. |
This looks like great step forward, thanks @eisenhauer. Let me mention @rmchurch since he also independently ran in to related issues a little while ago. For reference, there's this issue with more of my thoughts back in the day: #1661 For the use cases, I have, this PR would do all that's needed -- the case I have are Fortran-order arrays that I'm using in C++. I also think it's not easily possible to have more fine-grained control in the existing BP3/BP4 formats, and so I think it's fine to have a one ordering per file limitation there. (It might be in theory possible to get finger control by (ab)using progress groups, but that doesn't seem worth it.) In general, though, and that's why I mentioned it for BP5, I think it'd be useful to have the capability to specify the ordering for each dataset (ie., definitely per-variable, maybe even per-variable per-step). I don't think it's urgent to implement a corresponding API for it right now -- but if it's something that's not supported by the file format, it won't be possible to add it later without compatibility issues. Here are some examples why I find it's useful:
In terms of API, I think what you did can naturally be extended, and remains backward compatible:
So the only additional piece is a new API function like |
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.
Thanks Greg for the big effort!
I'm going to go ahead and merge this, as it passes all existing tests. |
@germasch Please look to see if this would reference the concerns you raise about array ordering in #2759.
@pnorbert, @JasonRuonanWang, et al. I think we have a notorious shortage of cross-language tests, so perhaps this needs extra attention before merge. The basic approach is minimalistic. There's a new parameter to DeclareIO that has an optional value specifying the array element ordering. The default is Auto, meaning that it's determined by the language specified when the ADIOS object is created, but it can also be RowMajor or ColumnMajor. This does not change the format of BP3/4 files on disk, where the language specified serves as a stand-in for the array ordering. Instead we store C++ for RowMajor and Fortran for ColumnMajor.