-
Notifications
You must be signed in to change notification settings - Fork 126
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
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.
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.
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; | ||
} |
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.
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);
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.
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.
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.
Makes sense, it might depend on the operator how we parse this, I will just pass the MinBlocksInfo
to the math function.
a48dff0
to
911a97c
Compare
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.) |
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 |
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).