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

Fix for the issue 3503. #3519

Conversation

dmitry-ganyushin
Copy link
Contributor

Python 1D arrays of size N are presented internally as C++ (1, N) structures. That made dataman serializer failed by reading.
Clean-up for PR #3515

@eisenhauer
Copy link
Member

So, riddle me this: You say that 1-D arrays in python show up in ADIOS as 2-D arrays with the first dimension being 1. I'm willing to buy that. But then in the test you have code on the reading side that looks like this:
bufshape = recvar.Shape()
self.assertTrue(bufshape[0] == self.Nx)
That is, you've asked ADIOS for the dimensions of the array, and you assertTrue that the first dimension is not 1, but is instead self.Nx. Even if somehow Python presents 1D numpy arrays as 2-D to ADIOS, I'm pretty sure that there's no mechanism in the read side of the ADIOS python bindings for that "extra" first dimension with value 1 to disappear. That is, if ADIOS C++ on the writer side is really seeing an array with shape [1, self.Nx] in the Put(), I don't think there's any way that the recvar.Shape() on the reader side would return anything other than [1, self.Nx]. This makes me wonder if you're successfully recreating the original problem. (Maybe I'm wrong, but if so I'd love to hear how that extra dimension disappeared.)

@dmitry-ganyushin
Copy link
Contributor Author

The original problem was reported for 1D numpy arrays presented as (1, N) shape. That is accepted at least for BP4 engine but did not work for DataMan. Added tests for both (N) and (1,N) shapes.

Copy link
Member

@eisenhauer eisenhauer left a comment

Choose a reason for hiding this comment

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

Just wanted to add back the original requests, target this to the release_29 branch, make the tests run with CI, and don't put them in as python bindings tests, but as dataman engine tests.

@dmitry-ganyushin
Copy link
Contributor Author

If I need to move python tests to other DataMan tests, I need to make it systematically for all python tests in bindings/python folder: they just duplicate some functionality tests like BP4 read/write or step selection using python. I think this change requires a separate PR.

After changes will come to the master branch, I can put the pick them into the release branch.

@eisenhauer
Copy link
Member

Bindings tests almost always duplicate at least some functionality in the core tests. But this is specifically to test a bug fix in the core of dataman. You used python because it was convenient, not because you were testing the python bindings. If it is really a bug in dataman, you could just as easily have created a C++ test to exercise it (but you wouldn't have put that test in the C++ bindings tests because of that).

@eisenhauer
Copy link
Member

(And we're targeting things that we want to be in the release to the release_29 branch because that makes life easier on Vicente, who then only has to move things in one direction after the release is cut. Yes, it could go to master and then to release_29, but it's easier to be consistent with everything else.

Comment on lines 20 to 25
python_add_test(NAME Test.Engine.DataMan1D.Serial SCRIPT TestDataMan1D.py)
python_add_test(NAME Test.Engine.DataMan1xN.Serial SCRIPT TestDataMan1xN.py)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you just need to put these lines in a if(ADIOS2_HAVE_Python) condition. If you look at the cdash output, you'll see that python isn't turned on in the nvhpc build: https://open.cdash.org/build/8527861

@eisenhauer
Copy link
Member

I think the code looks good now, but as per the conversation in the dev meeting, can we get this rebased on release_29 and the PR targeted to that branch too?

@vicentebolea
Copy link
Collaborator

@dmitry-ganyushin you do not need to open new Pull-requests to have this also in release_29. You can change this PR to target release_29, and once merged I can do the backport onto master.

Note that you will then have to rebase onto release_29 branch, the git command will be: git fetch origin; git rebase origin/release_29

@dmitry-ganyushin dmitry-ganyushin changed the base branch from master to release_29 March 9, 2023 19:38
Copy link
Member

@eisenhauer eisenhauer left a comment

Choose a reason for hiding this comment

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

My comments are addressed. I'll let Vicente do final approval.

@vicentebolea
Copy link
Collaborator

My comments are addressed. I'll let Vicente do final approval.

The latest rebase brings master only changes here. I will help Dmitry tomorrow to reorganize the commits. OTher than that it looks good to me

@vicentebolea vicentebolea force-pushed the b3503-dataman-1D-python-arrays-fin branch 2 times, most recently from 0154f4c to 1b22565 Compare March 10, 2023 19:26
@vicentebolea vicentebolea force-pushed the b3503-dataman-1D-python-arrays-fin branch from 1b22565 to a0d4586 Compare March 10, 2023 19:30
@dmitry-ganyushin dmitry-ganyushin merged commit 4ee931e into ornladios:release_29 Mar 20, 2023
@dmitry-ganyushin dmitry-ganyushin deleted the b3503-dataman-1D-python-arrays-fin branch March 20, 2023 15:30
vicentebolea pushed a commit that referenced this pull request Mar 20, 2023
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.

4 participants