hightower8083 / synchrad

Synchrotron Radiation calculator via openCL
GNU General Public License v3.0
18 stars 11 forks source link

Mismatched dimensions of z and t in tracksfromOPMD #32

Closed ronandoherty1 closed 3 weeks ago

ronandoherty1 commented 4 weeks ago

Hi there, hope you are well

I have been using the tracksfromOPMD function, and have been noticing that when I pass the "z_is_xi=True" argument, I sometimes get this error: "File "/home/dohertyr/.conda/envs/ff_betatron_sims/lib/python3.12/site-packages/synchrad-0.0.3-py3.12.egg/synchrad/converters.py", line 128, in tracksFromOPMD f[f'tracks/{i_tr:d}/z'] = z + c * t ^~~~~ ValueError: operands could not be broadcast together with shapes (29,) (33,) "

I realised that this error stems from the fact that, while the position and momentum variables are truncated according to their validity, the t values are not truncated. Thus, if there are any invalid values of the positions or momenta, then we get this mismatching of dimension.

I wrote up a fix to this which is here

https://github.com/ronandoherty1/synchrad/blob/dev/synchrad/converters.py

The idea is just that the function "split_tracks_by_nans" now outputs a list of "good_inds" as well, ie the indices for which the position and momenta are valid. Then the t array is truncated according to these indices, so dimension mismatching between x and z no longer occurs.

I hope this is helpful in improving the software. I would be happy to submit a pull request if you think this is a good fix.

hightower8083 commented 3 weeks ago

Hi @ronandoherty1

Thank you a lot for catching this issue!

Indeed z_is_xi option has not been thoroughly tested, in particular, I've never used it with the discontinuous tracks since they are more commonly specific to the time-domain PIC, and this feature addresses the case of QSA simulations (notably HiPACE), where z coordinate is a QSA variable and doesn't reflect the propagation, and in HiPACE, the beam particles usually do not appear/disappear.

But you are right and indeed when we split the track the time array is not splitted accordingly, so this should be fixed.

However, I think it is possible make less modifications and to avoid introducing this new variable good_inds. In fact each track already has the it_start (last value in the track container)., that defines its starting position with respect to the global time array. Therefore, it_start and the track length (or fragment) should be enough to get the part of t that corresponds to this particular track. For example, if here we replace t with t[it_start:it_start+z.size], it should also fix the problem.

Can you check with your data if this single-line fix works correctly, and let me know? And of course you are welcome to submit the PR so it'll be easier to follow and finalize the fix

ronandoherty1 commented 3 weeks ago

Hi @hightower8083,

Thanks for your response!

This 1 line fix did indeed correct the issue, and is certainly more elegant. I submitted a pull request with it.

Thanks again!

hightower8083 commented 3 weeks ago

thanx @ronandoherty1 looks good to me, so it's merged. Have fun using the code, and let me know of any problems or suggestions!