Fix RSPHERE Cartesian particle position conversion

[tomzhu0225]

Summary

• correct Cartesian-to-RSPHERE particle elevation initialization to use atan2(z, r_xy)
• store the computed spherical radius instead of the input Cartesian x coordinate
• add a component-wise regression for an equatorial radial point and a signed off-axis point

WarpX documents RSPHERE phi as elevation relative to the x-y plane. The previous conversion used atan2(r_xy, r), and the position insertion path stored x as the radius. For (x,y,z)=(0.5,0,0), this reconstructed (0.353553,0,0.353553) instead of the original point.

Validation

• test_rsphere_particle_coordinate_conversion.run
• test_rsphere_particle_coordinate_conversion.analysis

Both pass in the RSPHERE release build. The regression also covers (0.3,0.4,-1.2) so radius, azimuth, elevation sign, and all reconstructed Cartesian components are checked independently.

[dpgrote]

This section should be:

#elif defined(WARPX_DIM_RCYLINDER) || defined(WARPX_DIM_RSPHERE)
    pinned_tile.push_back_real(PIdx::r, r.data(), r.data() + np);
    amrex::ignore_unused(y,z);
#endif

The same line works for both sphere and cylinder.

[tomzhu0225]

Good catch, thanks! Updated to use the computed radial coordinate r for both RCYLINDER and RSPHERE.

[JustinRayAngus]

I agree with the bug fix but I'm not sure this needs an independent CI test. @EZoni

[EZoni]

@JustinRayAngus

Yes, I think we had a discussion with some in the @BLAST-WarpX/warpx-technical-committee, e.g., @roelof-groenewald and @RemiLehe, and we seemed to agree that a bug fix PR generally should not include an ad-hoc test — certainly not an integration (i.e., not unit) test anyways. We are including such guidance in #7155.

@tomzhu0225

Unless others in @BLAST-WarpX/warpx-technical-committee have objections, we would probably ask you to remove the integration test added in this PR and just keep the bug fix for now.

[lucafedeli88]

@JustinRayAngus

Yes, I think we had a discussion with some in the @BLAST-WarpX/warpx-technical-committee, e.g., @roelof-groenewald and @RemiLehe, and we seemed to agree that a bug fix PR generally should not include an ad-hoc test — certainly not an integration (i.e., not unit) test anyways. We are including such guidance in #7155.

@tomzhu0225

Unless others in @BLAST-WarpX/warpx-technical-committee have objections, we would probably ask you to remove the integration test added in this PR and just keep the bug fix for now.

I think that, ideally, a bug fixing PR should improve an existing test in such a way that the bug would have been caught by the test. If the bug concerns a completely untested feature, a separate PR to implement a new comprehensive test should be considered.

@EZoni mentioned unit tests, which I think we should adopt in WarpX, although it's not always easy (it may be easier to start with ablastr) .

[JustinRayAngus]

@EZoni @lucafedeli88 @dpgrote

It is my recommendation that a CI test is not needed here.

[tomzhu0225]

Thanks for the clarification, I've removed the standalone integration test.

[JustinRayAngus]

Thanks!