-
Notifications
You must be signed in to change notification settings - Fork 225
FieldTmp: Multiple Slots #1703
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
FieldTmp: Multiple Slots #1703
Conversation
5d0b3cd to
5597e48
Compare
| //! how many bytes for buffer is reserved to communication in one direction | ||
| constexpr uint32_t BYTES_EXCHANGE_X = 4 * 256 * 1024; //4 MiB | ||
| constexpr uint32_t BYTES_EXCHANGE_Y = 6 * 512 * 1024; //6 MiB | ||
| constexpr uint32_t BYTES_EXCHANGE_Z = 4 * 256 * 1024; //4 MiB |
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.
A little off-topic but just because I'm seeing it right now: in many memory.param files of the different examples there are different sizes of memory that need to be reserved for exchange but the comments often read the same numbers.
Just for consistency we could remove the comments there and state in the beginning that the unit used there is Byte.
Here at this point it would be 1 rather than 4 MiBs, right?
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.
different sizes of memory
that is generally speaking ok, since each example has different particle migration patterns resulting in different requirements for exchange buffers copying those per time step.
Just for consistency we could remove the comments there and state in the beginning that the unit used there is Byte.
great idea! open a separate issue or directly a PR for that?
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.
I can do that if you want, after yours is through - if you don't wanna rebase. But since this is such a minor change which only affects the comments I'd also be fine if it happened in the same PR.
There were some more typos in parts of the code that you didn't specifically touch in the files. Here to be precise, and in the comment below where it says negativ instead of negative. But a separate commit would be enough for this.
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.
yep, not touched, saw those too.
|
Great! 😄 ✨ |
| DataConnector &dc = Environment<>::get().DataConnector(); | ||
| FieldTmp& fieldTmp = dc.getData<FieldTmp > (FieldTmp::getName(), true); | ||
|
|
||
| PMACC_CASSERT_MSG( |
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.
I like how with this implementation some overhead from the Particles class is removed.
Do I understand correctly, though, that every time that I would instatiate a FieldTmp object I would have to add this cassert, right? I guess this could not just be moved into the constructor?
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.
every time you want to run dc.getData you assume the slot you access is really allocated in the current simulation. the assert checks that the user did not trick you by not allocating them and that getData() will not throw at RT. I just added it since it's a CT user param currently, so CT checking makes sense.
|
thanks for the lovely usage example 👍 |
5597e48 to
da203c4
Compare
| fieldTmp->getHostDataBox().getPointer()); | ||
|
|
||
| dc.releaseData(FieldTmp::getName()); | ||
| dc.releaseData( FieldTmp::getName() + "0" ); |
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.
please use FieldTmp::getUniqueId( 0 )
| ValueType()); | ||
|
|
||
| dc.releaseData(FieldTmp::getName()); | ||
| dc.releaseData( FieldTmp::getName() + "0" ); |
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.
please use FieldTmp::getUniqueId( 0 )
da203c4 to
bc89360
Compare
Expose a user input variable `fieldTmpNumSlots` which defines how many `FieldTmp` scalar fields will be needed. This defaults as before to `1` additional field.
bc89360 to
eb781d8
Compare
|
@psychocoderHPC review comments updated! :) @n01r if you want, you can write a kernel using two FieldTmp quantities now for testing the infrastructure is ready for TF. |
|
RT test: oh oh, segfaults with 2 slots enabled and standard I/O of FieldTmp quantities.... |
eb781d8 to
9b37b76
Compare
|
Thx for the update! I will rebase on that and continue implementing. 2016-12-22 Update: The first run with the two new fields was successful! I only have to calculate the actual "temperature" and density values and we're in the Thomas-Fermi business! 😛 |
Allows to use multiple
GridBuffers("slots") inFieldTmpfor all operations done with it so far.Required for, e.g., T_e and n_e in Thomas-Fermi Ionization.
Current control: the number of scalar temporary fields can be set in
memory.paramvia:Still throws dozens of__host__ __device__warnings but might compile already.Thanks to @psychocoderHPC for pair programming support.
Usage
To Do