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

Named SoA Support #4163

Merged
merged 10 commits into from
Oct 9, 2024
Merged

Named SoA Support #4163

merged 10 commits into from
Oct 9, 2024

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Sep 23, 2024

Summary

Add support to optionally name SoA Real and Int components, both for compile-time and runtime components.

  • define and defaults
  • single tile Get*Data(std::string)
  • refactor out default name generation for reuse, finalize defaults in AddReal/IntComp
  • test

Additional background

Close #3614

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • 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

{
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

A stack address which arrived via a
parameter
may be assigned to a non-local variable.
{
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

A stack address which arrived via a
parameter
may be assigned to a non-local variable.
Add support to optionally name SoA Real and Int components, both
for compile-time and runtime components.
Comment on lines 68 to 87
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));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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/AddIntComp without name argument

@atmyers
Copy link
Member

atmyers commented Oct 3, 2024

@ax3l I think this is ready now - want to give it a review?

{
AddRealComp(getDefaultCompNameReal<ParticleType>(NArrayReal+m_num_runtime_real), communicate);
Copy link
Member Author

Choose a reason for hiding this comment

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

zero indexed and used before m_num_runtime_real++, looks correct.

Copy link
Member Author

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +29 to +30
std::vector<std::string>* soa_rdata_names=nullptr,
std::vector<std::string>* soa_idata_names=nullptr
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, 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?

Comment on lines +303 to +305
// 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;
Copy link
Member Author

Choose a reason for hiding this comment

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

We could own the names here and let PC and ParticleTile reference them...

@ax3l
Copy link
Member Author

ax3l commented Oct 9, 2024

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 :)

@atmyers atmyers merged commit 8df11b6 into AMReX-Codes:development Oct 9, 2024
58 of 59 checks passed
@ax3l ax3l deleted the topic-named-soa branch October 9, 2024 18:56
atmyers pushed a commit that referenced this pull request Oct 10, 2024
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Named Particle SoA Components
2 participants