Add multi-kickmap store to InsertionDeviceKickMap

[gubaidulinvadim]

Adds an interface for storing and selecting between multiple named kickmaps on a single InsertionDeviceKickMap element. This supports workflows where the same ID element must represent different operating modes (e.g. different gap settings or modes (LV/LH/LT/Circular/Elliptical) without recreating the element.

New public API

• add_kickmap(key, nslice, fname, norm_energy) Reads a kickmap from a file or dict and stores it under a string key without activating it.
• use_kickmap(key) Activates a stored kickmap: swaps xtable, ytable, xkick, ykick, xkick1, ykick1, Nslice and Length to the data under that key. Raises KeyError if the key is not found.
• active_kickmap (property) Returns the key of the currently active kickmap, or None.
• list_kickmaps() Returns the list of stored keys, excluding internal metadata.

The store is held in a plain _kickmap_store dict attribute and is invisible to AT serialisation. PassMethod (IdTablePass / DriftPass) is intentionally untouched by use_kickmap.

This allows to do something like:

hu44.add_kickmap('LH', nslice=25, fname='HUS2_44_II10_g11500_Jaw0_kicks.txt', norm_energy=2.75)
hu44.add_kickmap('LV', nslice=25, fname='HUS2_44_II122000_g11500_Jaw0_kicks.txt', norm_energy=2.75)
hu44.add_kickmap('CR', nslice=25, fname='HUS2_44_II113250_g11500_Jaw0_kicks.txt', norm_energy= 2.75)
hu44.add_kickmap('LT', nslice=25, fname='HUS2_44_X112200_g11500_Jaw0_kicks.txt', norm_energy= 2.75)
hu44.list_kickmaps()
hu44.use_kickmap('LT')
hu44.active_kickmap

Also fix the docstring: the element supports first and second-order kick maps.

Tests

Add pyat/test/test_idtable_kickmap.py with 19 tests covering:

• store lifecycle (empty store, add, list, active key)
• field swap correctness and exact round-trip restore
• KeyError on missing or empty store
• internal _active sentinel not exposed to users
• add_kickmap does not activate
• PassMethod preserved across use_kickmap calls (both IdTablePass and DriftPass)
• set_DriftPass / set_IdTablePass still work after swapping
• kick arrays unchanged after DriftPass → IdTablePass round-trip
• full cycle: swap → Drift → IdTable → swap

Fixes #1105. @oscarxblanco, please have a look and tell me if you like the idea. I want a workflow where I just store the information in the lattice and can swap between different "modes" of the same ID.

Some tests are not very smart (changing the energy of an ID is not very physical, unless you have a ramped "storage" ring)

I would also note that readthedocs documentation of pyAT does not display some of the InsertionDeviceKickMap docstrings correctly.

[oscarxblanco]

@gubaidulinvadim , great ! At first glance it looks nice. It seems to provide the possibility to have different ID models. This is much more than what I understood you requested in #1105 (comment) about swapping tables.

[oscarxblanco]

why is the list of kick maps empty when the element is created ?

[oscarxblanco]

Also, why is the active_kickmap empty ?

[gubaidulinvadim]

Also, why is the active_kickmap empty ?

I've noticed this yesterday when testing. I need to fix this. The first kickmap (when object is created) is not registered at the moment. It's a bug.

[oscarxblanco]

I see that kickmaps are listed by name, what happens if one adds twice the same ID ? or if two different IDs had the same name ? or if the name is empty ?

[oscarxblanco]

I think that listing the IDs by name leads to several issues. Are you sure this is OK ?

[oscarxblanco]

saving the ring in .mat format looks incompatible with _kickmap_store. Could you check this ?

[gubaidulinvadim]

I think that listing the IDs by name leads to several issues. Are you sure this is OK ?

What do you propose instead? The kickmap filename is also not always unique (and not always intelligible for beam dynamics people). And the "name"/key are up to the user to define however they like it. At SOLEIL, we would typically have names with different polarisations or different gap value. But I'm open to suggestions on how to improve this idea.

P.S I can fix any bugs you find tomorrow.

[oscarxblanco]

I think that listing the IDs by name leads to several issues. Are you sure this is OK ?

What do you propose instead? The kickmap filename is also not always unique (and not always intelligible for beam dynamics people). And the "name"/key are up to the user to define however they like it. At SOLEIL, we would typically have names with different polarisations or different gap value. But I'm open to suggestions on how to improve this idea.

Here is my suggestion

• create a unique index for the table when the element is created or when adding a table.
• listing should display the index and name, where name is optional, i.e. it could be empty.
• the user should be able to select any of the tables through use_kickmap that accepts index or name. If the name appears more than once, it should raise a warning.

Also, I noticed that FamName is never updated, is this behaviour on purpose ? I think it is convenient, but, it comes with the risk of not knowing which is the active table in when working in matlab.

There might be other solutions, maybe someone else could advise (@lfarv , @swhite2401 ) ?

P.S I can fix any bugs you find tomorrow.

Ok, no problem.

[gubaidulinvadim]

I think the duplicate name is not possible currently because the name and kickmap parameters are stored in a Python dictionary. When you try to create a duplicate name, it overwrites the existing kickmap.

[oscarxblanco]

I would also note that readthedocs documentation of pyAT does not display some of the InsertionDeviceKickMap docstrings correctly.

Please, do the changes you consider adequate.

[gubaidulinvadim]

Also, why is the active_kickmap empty ?

why is the list of kick maps empty when the element is created ?

Now on object initiation, a 'default' kickmap is created and registered in kickmap list. The new behaviour sets it as active. So, you can see which kickmap is active and that the list of kickmaps only has a default one.

I see that kickmaps are listed by name, what happens if one adds twice the same ID ? or if two different IDs had the same name ? or if the name is empty ?

If you add twice the same key, for example 'LH' and 'LH' the old one will be overwritten. The kickmaps are stored in a simple Python dictionary. The keys of a dictionary are unique.

I think that listing the IDs by name leads to several issues. Are you sure this is OK ?

What do you propose instead? The kickmap filename is also not always unique (and not always intelligible for beam dynamics people). And the "name"/key are up to the user to define however they like it. At SOLEIL, we would typically have names with different polarisations or different gap value. But I'm open to suggestions on how to improve this idea.

Here is my suggestion

* create a unique index for the table when the element is created or when adding a table.

* listing should display the index and name, where name is optional, i.e. it could be empty.

* the user should be able to select any of the tables through `use_kickmap` that accepts index or name. If the name appears more than once, it should raise a warning.

Also, I noticed that FamName is never updated, is this behaviour on purpose ? I think it is convenient, but, it comes with the risk of not knowing which is the active table in when working in matlab.

There might be other solutions, maybe someone else could advise (@lfarv , @swhite2401 ) ?

P.S I can fix any bugs you find tomorrow.

Ok, no problem.

I do not like an index approach. If someone defines four kickmaps without a name and they are assigned an index, then there's no way for the user to know what each index means (gap value? phase value? polarisation?). But we can implement it if everyone agrees that this is necessary.

For Matlab, I do not know anything because I do not intend to ever use it :) Do you think the FamName should always change to reflect that the kickmap is different? I'd imagine changing FamName dynamically is undesirable because it should serve as an ID for the element. I was imagining that an interface similar to mine here can be implemented in Matlab.

saving the ring in .mat format looks incompatible with _kickmap_store. Could you check this ?

I'm working on this and loking for a solution for Matlab formats. There's an issue with saving and loading lattice in .mat and .m formats. The kickmap dictionary is not saved or loaded properly and the information is lost for all kickmaps but the currently active one (it gets loaded as "default"). No issue with JSON format, which seems to be more powerful.

[oscarxblanco]

Dear @gubaidulinvadim , typically names are optional in AT. Empty names are allowed for many other elements.

It may be that in your case all kickmaps have a non-empty and unique strings to be used as names, but, it would be unusual in AT to force it.

There might be another alternative, e.g. if name is not given assign the filename as the kickmap name. This is also a bit risky because you will have to deal with spaces and special characters.

[gubaidulinvadim]

Dear @gubaidulinvadim , typically names are optional in AT. Empty names are allowed for many other elements.

It may be that in your case all kickmaps have a non-empty and unique strings to be used as names, but, it would be unusual in AT to force it.

There might be another alternative, e.g. if name is not given assign the filename as the kickmap name. This is also a bit risky because you will have to deal with spaces and special characters.

Right now the name is always assigned as 'default'

Yes, in the end this is maybe a better solution. I've managed to fix the saving and loading in .m and .mat formats (which have limitations not present in JSON). There's a hack with 100+ lines of code that saves everything as arrays and reconstructs things... I'm not very happy with it. I will try to see if indexing it instead can help.

Compare commits e7d4353 (.m, .mat and .json is working) and b4ebb16 (.json is working; .m and .mat are not working). @swhite2401 @lfarv maybe some of you would know a better solution? Or maybe there's a solution on the side of saving and loading the AT lattice. I'm not really familiar with MATLAB formats or their limitations.

[swhite2401]

@gubaidulinvadim I am just picking up on this.
Could you quickly summarize where help is needed, is just the file saving to .mat .m files?
Can this proposal as it is for testing?

[gubaidulinvadim]

@gubaidulinvadim I am just picking up on this. Could you quickly summarize where help is needed, is just the file saving to .mat .m files? Can this proposal as it is for testing?

Yes, the branch can be tested as is. Either of the last two commits can be tested.

• b4ebb16 original fix that doesn't work for .m and .mat files
• e7d4353 solution that I came up with to fix saving to .m and .mat

The main issue is saving in .m and .mat files. The new interface can store several kickmaps in an Python dictionary for a single object

hu44.add_kickmap('LH', nslice=25, fname='HUS2_44_II10_g11500_Jaw0_kicks.txt', norm_energy=2.75)
hu44.add_kickmap('LV', nslice=25, fname='HUS2_44_II122000_g11500_Jaw0_kicks.txt', norm_energy=2.75)
hu44.add_kickmap('CR', nslice=25, fname='HUS2_44_II113250_g11500_Jaw0_kicks.txt', norm_energy= 2.75)
hu44.add_kickmap('LT', nslice=25, fname='HUS2_44_X112200_g11500_Jaw0_kicks.txt', norm_energy= 2.75)
hu44.list_kickmaps()
hu44.use_kickmap('LT')
hu44.active_kickmap

There's also a question if such an interface needs to be in Matlab or if Python-only changes are accepted.

[swhite2401]

There's also a question if such an interface needs to be in Matlab or if Python-only changes are accepted.

Python only changes are acceptable, we already have those for many things. This is true as long as you do not modify passmethod (C) which can be called by either interface, but looking at your PR this is not the case.

[swhite2401]

For the other problem, I first need to understand your code....

[gubaidulinvadim]

For the other problem, I first need to understand your code....

The original fix in b4ebb16 is much easier to understand :)

[swhite2401]

Ok, could you send me a kick map file for testing?

[gubaidulinvadim]

Ok, could you send me a kick map file for testing?

I've attached the files. The kickmaps do not correspond to anything in SOLEIL II project :)

HUS2_44_.zip

[swhite2401]

@gubaidulinvadim, @oscarxblanco , I am going to post questions / comments / suggestions along the way as I test this, here the first few (not necessarily from this PR but since this is the first time I use it here they are):

-If the idtable device becomes a list it should be possible to instantiate an empty element and then add tables or give a list of files (similar to a list or an array)
-Could you explicitly put all required input argument in the function signature otherwise these are not reflected in the API doc. Typically using *args is not helping the user
-The docstring is not very detailed, each positional and keyword argument should be described
_ from_user() shouldn't be private? Also the docstring is confusiong here: it looks like if should belong to the init() function of InsertionDeviceKickMap

• are set_DriftPass, set_IdTablePass and get_Passmethod really usefull? Can't you just read/set the Element attributes as for any other AT element?

But maybe I also misunderstood some of these things.... more to come

[oscarxblanco]

@swhite2401 , you are right about these observations. Most of them are the consequence of me not having much experience with AT when I created this element about 5 years ago, so, I added what I thought could be useful.

-If the idtable device becomes a list it should be possible to instantiate an empty element and then add tables or give a list of files (similar to a list or an array)

I don't think that having a list of tables inside one element is of any advantage wrt creating an element per table and changing the element.

However, this is @gubaidulinvadim 's idea and I think he develop it to have a simplified way of simulating several ID types and gaps, making of this element something closer to a library of IDs.

On the other hand, it could be a step to consider tracking when ID gaps are constantly changing.

-Could you explicitly put all required input argument in the function signature otherwise these are not reflected in the API doc. Typically using *args is not helping the user

Right, it may be done in this PR ? otherwise I can fix it after this one is merged.

-The docstring is not very detailed, each positional and keyword argument should be described
Right, I think it is just misplaced. I could reorganize it if not done in this PR.

_ from_user() shouldn't be private? Also the docstring is confusiong here: it looks like if should belong to the init() function of InsertionDeviceKickMap

from_user was supposed to provide a way to inspect a table without creating the element. Sometimes the tables are a long to display and the info is difficult to plot. This was a way to read the file and get the data without creating the element. But, I agree that the documentation is not helpful. I could re organize it if not done in this PR.

are set_DriftPass, set_IdTablePass and get_Passmethod really usefull? Can't you just read/set the Element attributes as for any other AT element?

I thought they might be useful instead of manipulating the pass method names directly, but, this was just my lack of experience with AT. It was difficult to memorize and write many times the correct pass method per element with and without radiation, active or identity or drift, so, I decided to create something that sets the pass method name instead of having to type it every time.

[gubaidulinvadim]

Most of your questions are for the original implementation of @oscarxblanco. I don't mind making the changes in this PR to avoid handling the conflict between changes.

@gubaidulinvadim, @oscarxblanco , I am going to post questions / comments / suggestions along the way as I test this, here the first few (not necessarily from this PR but since this is the first time I use it here they are):

• If the idtable device becomes a list it should be possible to instantiate an empty element and then add tables or give a list of files (similar to a list or an array)

Yes, we can do that and instantiate an ID without a kickmap that will have DriftPass PassMethod.

• Could you explicitly put all required input argument in the function signature otherwise these are not reflected in the API doc. Typically using *args is not helping the user

@oscarxblanco feel free to change the doc, I can also do it in this PR.

• The docstring is not very detailed, each positional and keyword argument should be described _ from_user() shouldn't be private? Also the docstring is confusiong here: it looks like if should belong to the init() function of InsertionDeviceKickMap

@oscarxblanco feel free to change the doc, I can also do it in this PR. from_user wasn't private originally. I was also thinking that it's only ever used in the init. We can make it public if necessary. My idea was that add_kickmap() kind of replaces from_user.

* are set_DriftPass, set_IdTablePass and get_Passmethod really usefull? Can't you just read/set the Element attributes as for any other AT element?

This is from Oscar, but I find it useful. For IDs, one needs to set it to drift to study the impact of an ID and having a method for it is handy. get_Passmethod is maybe not so necessary.

But maybe I also misunderstood some of these things.... more to come

The idea of storing several tables is to have all necessary information in the lattice. And I find it a bit annoying having to create N different objects and swap them in a lattice by hand. But this is maybe my lack of experience with pyAT.

[oscarxblanco]

@gubaidulinvadim , I will wait until this PR is closed to make any change that is left pending.

[swhite2401]

Ok thanks for the clarifications!

I am also biased by my usage.... I realize that it is not so easy for first time user to disable an element.
So you think a generic .enable() / .disable() function for the Element object would be useful?

It would replace the passmethod by DriftPass or IdentityPass depending on the length. It is rather generic and would prevent have specific function like you proposed in this element.

I haven't started looking at your problem though...

[gubaidulinvadim]

Ok thanks for the clarifications!

I am also biased by my usage.... I realize that it is not so easy for first time user to disable an element. So you think a generic .enable() / .disable() function for the Element object would be useful?

It would replace the passmethod by DriftPass or IdentityPass depending on the length. It is rather generic and would prevent have specific function like you proposed in this element.

I haven't started looking at your problem though...

I think a standard enable/disable is good.

[swhite2401]

Hi both,

So I had a quick and the problem seems to come from private attributes, starting with "_", _kickmap_store and _active are concerned.

You may simply remove the underscore to make it work, otherwise, you can use _drop_attr of the element class to exclude them. This option works only if these attributes are re-generated at initialization of the object, which seems to be the case

[swhite2401]

I think a standard enable/disable is good.

Ok I will work on that

[gubaidulinvadim]

Hi both,

So I had a quick and the problem seems to come from private attributes, starting with "_", _kickmap_store and _active are concerned.

You may simply remove the underscore to make it work, otherwise, you can use _drop_attr of the element class to exclude them. This option works only if these attributes are re-generated at initialization of the object, which seems to be the case

I'll try it out and see if this is better than the current solution.

[swhite2401]

I'll try it out and see if this is better than the current solution.

You can also use _convert_attr, see line 72 of element_object.py, this is done for alignment errors attributes.
If you use this you have to make sure that the constructor uses keywords without the underscore.