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

Add MinBlocksInfo to BP5 writer engine #3794

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

eisenhauer
Copy link
Member

This PR is in support of the Derived Variables work and includes example usage in an inactive #ifdef in the BP5 writer engine. While it seems to be functional, the functions here won't be used until derived variable implementation is complete (or we introduce unit testing of some kind for this).

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.

Thanks Greg, this makes (mostly) sense (I am a bit confused why each block has a count in multiple dim). All we need is the MinBlocksInfo and GetPtr functions for our branch but we can merge the whole thing and I'll remove what we don't need. As you like.

I will approve as soon as I run this on my laptop/Summit later today.

source/adios2/engine/bp5/BP5Writer.cpp Show resolved Hide resolved
Comment on lines 509 to 524
for (const auto &varPair : vars)
{
auto baseVar = varPair.second.get();
auto mvi = MinBlocksInfo(*baseVar, m_WriterStep);
if (mvi)
{
std::cout << "Info for Variable " << varPair.first << std::endl;
PrintMVI(std::cout, *mvi);
if (baseVar->m_Type == DataType::Double)
std::cout << "Double value is " << *((double *)mvi->BlocksInfo[0].BufferP)
<< std::endl;
delete mvi;
}
else
std::cout << "Variable " << varPair.first << " not written on this step" << std::endl;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In EndStep my code parses the derived variables so I'll change it to use parts of this code to get the BufferP pointer for each block.

For the count, I assume the data is serialized and contiguous in the buffer but only for each block and not across multiple blocks. So, I will go over mvi.BlocksInfo and get the pointer for each block with the total number of elements (which I thought is given by blk.Count but from the print function it seems I need to sum the blk.Count[i]).

The type I assume I get from the variable?

auto type = ToString(varPair.second->m_Type);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the blocks info is really what we need to make sense of that Put() when we get it on the reading side, so the Count is essentially the N-dimensional Count given in the Put(). We also have the Shape of the object (once) and the Offset (or Start) values for each block. Both of those are Null for local arrays. But yes, if you just want number of elements in this block, you need to multiply the Count values. However, I'd assume that more sophisticated operators might depend on the dimensionality of the block, etc.

You do indeed get the type info from the variable (this is the VariableBase class, so common across all types). The original BP4 blocksinfo was templated, so you had to call the right GetBlocksInfo in order to even have blocks info, so the type was implied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, it might depend on the operator how we parse this, I will just pass the MinBlocksInfo to the math function.

@eisenhauer
Copy link
Member Author

The failure here is in Engine.BP./BPReadMultithreadedTestP.ReadFile/.BP5.Serial, a test that I see fail all the time on the "no rerun" test setup. That it failed here means that it failed 5 times in a row, which is pretty marked instability. @pnorbert it's probably time to take a look at this if we can. In the meantime, this failure is not relevant to this PR and won't prevent merging, so I think we're good to go after someone reviews. (Alternatively @anagainaru, if you just want to pull the bits that you need into your derived vars branch, that would be fine too.)

@anagainaru
Copy link
Contributor

Alternatively @anagainaru, if you just want to pull the bits that you need into your derived vars branch, that would be fine too.

Either way works for me. I need to rebase on the new master anyway so you might as well just merge this and I'll change the EndStep function

@eisenhauer eisenhauer merged commit 2322908 into ornladios:master Sep 11, 2023
29 of 30 checks passed
@eisenhauer eisenhauer deleted the WriterMinVar branch September 11, 2023 13:46
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