Named SoA Support #4163
Named SoA Support #4163
Conversation
ax3l
force-pushed
the
topic-named-soa
branch
from
15145a6 to
57ee5c2
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| m_defined = true; | ||
| m_runtime_rdata.resize(a_num_runtime_real); | ||
| m_runtime_idata.resize(a_num_runtime_int ); | ||
| m_rdata_names = soa_rdata_names; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory Warning
| { | ||
| m_defined = true; | ||
| m_runtime_rdata.resize(a_num_runtime_real); | ||
| m_runtime_idata.resize(a_num_runtime_int ); | ||
| m_rdata_names = soa_rdata_names; | ||
| m_idata_names = soa_idata_names; |
Check warning
Code scanning / CodeQL
Local variable address stored in non-local memory Warning
Add support to optionally name SoA Real and Int components, both for compile-time and runtime components.
ax3l
force-pushed
the
topic-named-soa
branch
from
57ee5c2 to
9986eef
Compare
| int first_r_name = 0; | ||
| // push back x,y,z | ||
| if constexpr (ParticleType::is_soa_particle) { | ||
| constexpr int x_in_ascii = 120; | ||
| for (int i=0; i<AMREX_SPACEDIM; ++i) | ||
| { | ||
| std::string const name{char(x_in_ascii+i)}; | ||
| m_soa_rdata_names.push_back(name); | ||
| } | ||
| first_r_name = AMREX_SPACEDIM; | ||
| } | ||
| for (int i=first_r_name; i<NArrayReal; ++i) | ||
| { | ||
| m_soa_rdata_names.push_back("real_comp" + std::to_string(i-first_r_name)); | ||
| } | ||
| for (int i=0; i<NArrayInt; ++i) | ||
| { | ||
| m_soa_idata_names.push_back("int_comp" + std::to_string(i)); | ||
| } | ||
|
|
There was a problem hiding this comment.
We discussed that we want to make this two functions (one for real, one for int) that take the ParticleType and index to return a default name on the current index.
This way, we can reuse this in the:
- current I/O logic (existing naming scheme we took over here)
- here
- in
AddRealComp/AddIntCompwithoutnameargument
|
@ax3l I think this is ready now - want to give it a review? |
| { | ||
| AddRealComp(getDefaultCompNameReal<ParticleType>(NArrayReal+m_num_runtime_real), communicate); |
There was a problem hiding this comment.
zero indexed and used before m_num_runtime_real++, looks correct.
There was a problem hiding this comment.
Very nice!
One thought on maybe moving the ownership around for safer function calls. Currently, there is a small risk someone builds their own StructOfArrays outside of a PC and then passes non-owned names into it, which would easily create memory violations when the SoA outlives the passed names.
| std::vector<std::string>* soa_rdata_names=nullptr, | ||
| std::vector<std::string>* soa_idata_names=nullptr |
There was a problem hiding this comment.
Yes, this is indeed not super save... Should we store a reference to a PC instead?
Or store the names in StructOfArrays and look them up when we need them from PC and ParticleTile?
| // names of both compile-time and runtime Real and Int data | ||
| std::vector<std::string>* m_rdata_names = nullptr; | ||
| std::vector<std::string>* m_idata_names = nullptr; |
There was a problem hiding this comment.
We could own the names here and let PC and ParticleTile reference them...
|
Discussed today: the case here is not very common, because we create SoAs from PCs and from other SoAs. In tiling, we actually create an SoA per tile in the grid of tiles, which if we store the string in those would duplicate the names quite a bit (yet, not as much as the particle arrays also owned by those SoA's). We conclude, it is unlikely to create issues right now in this design in the common usage :) |
## Summary Expose the names to public member functions. ## Additional background Needed for Python bindings. Follow-up to #4163 ## Checklist The proposed changes: - [ ] fix a bug or incorrect behavior in AMReX - [x] add new capabilities to AMReX - [ ] changes answers in the test suite to more than roundoff level - [ ] are likely to significantly affect the results of downstream AMReX users - [ ] include documentation in the code and/or rst files, if appropriate
Summary
Add support to optionally name SoA Real and Int components, both for compile-time and runtime components.
Get*Data(std::string)AddReal/IntCompAdditional background
Close #3614
Checklist
The proposed changes: