bugfix NxN ID table in atidtable_dat.m, update second order kick variables in C, include first order kick in pyat

[oscarxblanco]

This PR resolves issue #682 when reading an Insertion Device table in matlab through atidtable_dat where the kick maps are of N by N dimensions.

[oscarxblanco]

Dear all,

this is a bug fix to issue #683 where I reported an error when trying to load in matlab a kickmap with a square grid (N by N points per table). The function atidtable_dat needs only three values (L,Nx,Ny) from mhdrload_bis. However, mhdrload_bis returns data_mat which is of varying length depending on the table grid size.

The expression in https://github.com/atcollab/at/blob/52298a90a5059e0858298a1cfec2a0da3f12c143/atintegrators/mhdrload_bis.m#L158 divides length(data) which is (cols+rows(1+cols)), by cols. For a rectangular grid this is not a integer, the entire reshape is skipped and nothing is assigned to data_mat. But in the case of N=cols=rows, an NxN grid, the expression returns an integer which is a valid shape and more data is returned.

The proposed solution is to explicitly catch in atidtable_dat only the three required values (L,Nx,Ny).

    L=data_mat(1,1,1);
    Nx=data_mat(1,1,2);
    Ny=data_mat(1,1,3);

I add to this message two Insertion Device tables, one with a rectangular and one with a squared grid for verification, and two lines to execute a test.

[idtable_rectangulargrid.txt](https://github.com/atcollab/at/files/13186828/idtable_rectangulargrid.txt)
[idtable_squaregrid.txt](https://github.com/atcollab/at/files/13186829/idtable_squaregrid.txt)

utest1 = atidtable_dat('utest',10,'idtable_rectangulargrid.txt',3,'IdTablePass')
utest2 = atidtable_dat('utest',10,'idtable_squaregrid.txt',2.75,'IdTablePass')

o

[lfarv]

For me, the whole file scanning process looks much too complicated, and the problem here seems more in the behaviour of mhdrload_bis. But if this solves the problem, that's OK for me.

[oscarxblanco]

Dear @lfarv , I agree that mhdrload_bis is overly complicated for the task.

I tried to see what was the reason for it and it seems that the early development tried to adapt another script 'mhdrload' to the needs of the Insertion Device table. However, the formats are incompatible, the header in the insertion device file has a different format, and the values in the header do not coincide with the column and rows on the table.

At the end most of it fails and it only returns data when it is in format nxn which is the case of a single value in a line (a 1x1, i.e. 1row,1column data), and incidentally ID tables with a square grid.

Therefore, I decided to cherry-pick only the data strictly necessary, and ignore whatever other stuff is in the return from mhdrload_bis. This is a minimal intervention with no other goal than avoid the error.

I am not familiar with matlab text reading functionalities, and I have only a few examples of ID tables at hand. If there is a better way to read text files in matlab I could have a look for a replacement of mhdrload_bis, but, I would need more example to test.

[oscarxblanco]

Dear @lfarv , having a quick look it seems that importdata could do the job. It is already used in atidtable_dat where the kick tables are read by assuming fix header lengths.

If you consider this ok, I would replace mhdrload_bis with calls to importdata.

[oscarxblanco]

Dear @lfarv , I have removed the calls to mhdrload_bis. The three necessary values L,Nx and Ny are read through importdata.

[lfarv]

@oscarxblanco : I did not want to trigger large modifications, but just say that if there are in the future other decoding problems, it could be wise to review the whole thing, But for now, if this solution solves your problem, that's fine for me. If there are no comments from the other reviewers, I'm ready to approve.

[oscarxblanco]

Dear @simoneliuzzo , would you mind to review this Merge Request ?

I have made a change in the matlab method to readout ID table files assuming data is on specific rows. This was already the case of the previous implementation but done in a convoluted way, so, this modification does not add or loose any flexibility in the file format, it just make explicit the rigidity of the input file format.

If you have the time, and you still have some ID files at hand, could you try the following lines of matlab code and check that the ID is loaded ?

utest1 = atidtable_dat('utest',integrationslices,'somefilename',mybeamenergy,'IdTablePass')

[lnadolski]

Dear @orblancog,

Reviewing the code, I have several remarks and questions

• Using PhysConstant is preferable
• I would prefer, for consistency, to label kick1 and kick2 for the radiation kick maps of first order and second order.
• The file IdTablePass.c could be modified accordingly
• Have you checked the first order kick maps?

[oscarxblanco]

Dear @lnadolski , thank you for the suggestions and modifications you introduced. With respect to the first order kick map I haven't gone into enough detail up to now, but, @ZeusMarti has given me an example that contains first and second order kickmaps from measurements. I will have a look into it.

@ZeusMarti also mentioned he had the idea of using hdf5 file formats which might be richer, but, it seems that it also needs some interest from the ID modelling community due to possible incompatibilities of hdf5 file format with older versions of mathematica, and additional development in mathematica and python interfaces to Radia.

For pyat it would mean to include an hdf5 package (like h5py) on the dependency list, while for matlab I think it is already there but I would not know which is the oldest matlab version who supports it.

[lfarv]

I would not know which is the oldest matlab version who supports it.

h5read, h5write, and others were introduced in Matlab R2011a. AT requires R2016b, though it may mostly work on older versions. So no problem for HDF5

For python, no problem to add h5py as a dependency when it becomes useful.

For this PR, I let you and @lnadolski decide if the element attribute names must be changed, possibly breaking backward compatibility,

[oscarxblanco]

Dear @lnadolski , I believe it would be possible to keep backwards compatibility and implement the variable renaming you requested by supporting both kick and kick2 as inputs, and including info messages whenever both are present to make clear that kick2 has priority over kick. New files would be saved with the new variable names.

kick1 keeps its name, it should not be modified, but needs to be implemented in python.

I will have a look on the necessary changes.

[simoneliuzzo]

Dear @orblancog ,

I have tryed the code line you asked.

It gives me an error at line 76 of atidtable_dat.m, while parsing the file.

I attach here the kickmap file I am using for the test.

kickmap_w150_20mm.txt

I hope this is of help. let me know and I will test again.

[oscarxblanco]

Thank you @simoneliuzzo, now I know that in addition to spaces as separators, it could also be TABS.

[simoneliuzzo]

Dear @oscarxblanco,

here the output

[oscarxblanco]

Dear @simoneliuzzo, great ! Thank you for testing. The text reading function now supports either all TABs or all spaces as data separator. If you see a problem with any other file you have, please, let me know.

A few days ago I put this PR as work in progress because I am trying to make a backwards compatible modification of the variable name kick to kick2.

[lnadolski]

Concerning hdf5, this is good idea for the future, but for back compatibility, the present format should be maintained.

[oscarxblanco]

Dear @lnadolski ,

the first order kickmaps are now implemented in pyat as in AT matlab. The input file in pyat is a text file as before, only with two more optional kicktables (i.e. two or 4 tables are admitted). No other input formats are supported in pyat for the moment, while in AT matlab it could be a text file, or a mat file. The example files in at/atmat/atdemos/IDModelling has been changed accordingly with the new variable names.

These modifications also keep backwards compatibility. For python and matlab users there will be warnings when required, otherwise the name swapping is done on the code, and new files will contain kick1 and kick2 as requested. The biggest change was in the IdTablePass for matlab, because rings loaded through .mat files could only be checked once a calculation is started, while, in python the variable check is done while reading the input files.

I have added a folder with automatic tests for several input/output combinations between pyat and matlab, added tests for new file formats, and backwards compatibility.

You will need to configure the file setatversion.m, and have this branch of pyat installed. After that, you could launch bash dotestsaveID.sh. The test could be split in separated python and matlab calls if you prefer.

saveIDtest02a.zip

[oscarxblanco]

Dear @lnadolski ,

while checking the sign of the first order kick map I think there is a decision to take wrt to the convention. The implementation used thus far assumes same sign for both transverse kicks while the article by Elleaume explicitely says they should be opposite. We could either leave the code as it is and apply the difference in sign in the input table, or, implement in the code the equations as P. Elleaume wrote them and the sign of saved kick1 measurements should be inverted.

I have written it in more detail here https://github.com/atcollab/at/discussions/703

Please, other Insertion Device users, (@ZeusMarti , @simoneliuzzo , others), feel free to comment.

o

[oscarxblanco]

Dear all, over the last month I have not received any comments wrt to the sign convention for the first order kick map. Therefore, I have decided to remove the signs from the normalization factor, and therefore, the input tables will need to include all signs in the Equations (3-5) of P. Elleaume's article, mentioned in this discussion : https://github.com/atcollab/at/discussions/703.

Best regards, o

[oscarxblanco]

I think this could be reviewed.

[oscarxblanco]

Dear @lnadolski and @lfarv ,

I will close this Pull Request and open smaller ones.

It seems that it started from a small modification and ended on many additions, making it complicated to review.

Best regards, o