Fix conduit for changes to particles #1010
Conversation
| @@ -64,18 +64,21 @@ ParticleTileToBlueprint(const ParticleTile<NStructReal, | |||
| // point locations from from aos | |||
| //----------------------------------// | |||
|
|
|||
| n_coords["values/x"].set_external(const_cast<ParticleReal*>(&p_aos.pos(0)), | |||
| const ParticleReal* xp = pstruct.data(); | |||
There was a problem hiding this comment.
| const ParticleReal* xp = pstruct.data(); | |
| const ParticleReal* xp = p_struct.data(); |
| num_particles, | ||
| 0, | ||
| struct_size); | ||
| #if AMREX_SPACEDIM > 1 | ||
| n_coords["values/y"].set_external(const_cast<ParticleReal*>(&p_aos.pos(1)), | ||
| const ParticleReal* yp = pstruct.data() + 1; |
There was a problem hiding this comment.
| const ParticleReal* yp = pstruct.data() + 1; | |
| const ParticleReal* yp = p_struct.data() + 1; |
| num_particles, | ||
| 0, | ||
| struct_size); | ||
| #endif | ||
| #if AMREX_SPACEDIM > 2 | ||
| n_coords["values/z"].set_external(const_cast<ParticleReal*>(&p_aos.pos(2)), | ||
| const ParticleReal* zp = pstruct.data() + 2; |
There was a problem hiding this comment.
| const ParticleReal* zp = pstruct.data() + 2; | |
| const ParticleReal* zp = p_struct.data() + 2; |
|
I left some comments in the code, i.e., there are a couple typos |
|
I don't have much ability to test this on my end - could you let me know whether the current code works for you? We could also chat on the WarpX slack, if that's easier. |
|
I understand the testing problem. I am happy to iterate with you to get this working. You can invite me under larsen30@llnl.gov. Probably faster on slack. |
|
I have been pushing on this as well, I don't fully understand the implications of the recent changes how the particle containers are represented. Basically, since we expect AOS, we need to access the pointer of the first of each of the entries we want to wrap (pos,id,cpu, the real data entries and the integer data entires). But it's not clear how to do this any more -- I am stuck getting rvalues back, which undermines getting the addresses. |
|
The current implementation of the particle class in AMReX uses an anonymous struct, which isn't allowed in the standard. As work towards removing it, basically I made some changes that broke the way this file was accessing the data pointers. @mclarsen and I iterated on this I think the current version of this pull request fixes the issue. Could you let me know and if so I'll merge it? Thanks! |
| const RealType* data () const { return &(m_data[0].pos(0)); } | ||
|
|
||
| const RealType* data () const { return &(m_data[0].m_rdata.pos[0]); } | ||
| RealType* data () { return &(m_data[0].pos(0)); } |
There was a problem hiding this comment.
might need an adjustment for the non-const case as well?
There was a problem hiding this comment.
The non-const actually works as is, the issue is that the pos method of particle returns a reference in the non-const case, but a value in the const case. So when you try to take the address of a value, you get the rvalue compile errors.
Perhaps that version should return const RealType&, but we might need to be careful about possible performance implications.
There was a problem hiding this comment.
ok, thanks for clarifying -- that helps me better understand the const changes as well.
|
Do you have any experience building Ascent tests in a Github CI? That might help us catch issues like this in the future before they are merged. |
|
We do, and I want it to test integrations. The issue is that it takes a long time to build all the dependencies. One solution is looking into caching the binaries(somehow) and building codes against it. Another alternative is to use our internal gitlabs CI, but that does not allow us to publish external results. Its relatively easy to setup and could test the integrations nightly against your development branch. Maybe @cyrush has some suggestions. |
|
That said, building conduit is far less involved than Ascent. Probably much easier to build/test. |
|
Testing with just conduit would be much easier and maybe a good first step. Ultimately -- I think we really want testing with Ascent. I'll take a look the AMReX CI setup and work to find a path that could work. |
This fixes a bug introduced in #963.