Variables

[lfarv]

This replaces #603 and #696. Upon request, #696 was split in 4 parts, here is the first one.

We introduce standard ways of parametrising a lattice and varying any quantity.

Variables

Variables are references to any scalar quantity. AT includes two predefined variable classes referring to scalar attributes of lattice elements:

• an ElementVariable is associated to an element object, and acts on all occurences of this object. But it will not affect any copy, neither shallow nor deep, of the original object,
• a RefptsVariable is not associated to an element object, but to an element location in a Lattice. It acts on any copy of the initial lattice. A ring argument must be provided to the set and get methods to identify the lattice.

Variable referring to other quantities may be created by:

• Deriving the Variable base class. Usually this consist in overloading the abstract methods _setfun and _getfun,
• Using the CustomVariable class, accepting user-defined get and set functions.

The documentation for this branch is available here.

[lfarv]

The new classes introduce name conflicts with existing Variable, ElementVariable classes. Keeping two slightly different objects is not desirable, so the idea is to smoothly replace the old classes with the new ones. The proposed strategy is:

1. Starting with this PR, the access to the new classes and functions is:

   from at.future import ElementVariable, ...

   Nothing changes for the old classes, still available with:

   from at import ElementVariable, ...

   or imply using at.ElementVariable(...).

2. At some point (version 1.0?), we will switch the default to the new scheme, and the old one will need something like:

   from at.deprecated import ElementVariable, ...

[lfarv]

Still trying to answer @simoneliuzzo's questions, concerning parameters:

I think that this is the natural user expectation

p5 = Param(-0.3, name='QD strength')
qd1.PolynomB[1] = p5

This is a major point. I couldn't find any solution such that a numpy array could turn itself into a ParamArray. But since I agree with Simone's that it is really frustrating, I am considering another solution: each array attribute of an Element would be initially set as a ParamArray at element creation. The performance penalty should be very small, I'll prepare this modification.

[lfarv]

Directly assigning a parameter to an array item is now allowed:

p5 = Param(-0.3, name='QD strength')
qd1.PolynomB[1] = p5

[lfarv]

Documentation largely improved, please check again…

[T-Nicholls]

The performance penalty should be very small, I'll prepare this modification.

Hello @lfarv, I would encourage you to do your own timing tests; however, when I ran my standard linopt timing script, I found this branch to be roughly between 2 to 3.85 times slower than the master. To me that is much too large of a slowdown, especially when I was already already planning to try and make future speed improvements to linopt due to its current speed.

Full timing data for reference:

master branch:
Mean time per linopt call over 1000 iterations (get_chrom = False):
    No refpts:
        Uncoupled: 0.046473862171173
        Coupled:   0.046404954195022
    All refpts:
        Uncoupled: 0.060675595998764
        Coupled:   0.143170256614685
Mean time per linopt call over 1000 iterations (get_chrom = True):
    No refpts:
        Uncoupled: 0.069423148155212
        Coupled:   0.069539334297180
    All refpts:
        Uncoupled: 0.084518674612045
        Coupled:   0.167194108247756

variables_and_parameters branch:
Mean time per linopt call over 1000 iterations (get_chrom = False):
    No refpts:
        Uncoupled: 0.174603536367416
        Coupled:   0.176802119970321
    All refpts:
        Uncoupled: 0.208549186229705
        Coupled:   0.290254921436309
Mean time per linopt call over 1000 iterations (get_chrom = True):
    No refpts:
        Uncoupled: 0.267525310277938
        Coupled:   0.269076839447021
    All refpts:
        Uncoupled: 0.301143751859664
        Coupled:   0.383905014276504

[lfarv]

@T-Nicholls : you raise a very important point. I tried to go more into the details of timings, down to the root of the problem which is the modification of the access to the attributes of elements.

There are 3 different test cases:

1. the master branch
2. the variables_observables branch which was the topic of the defunct #696 PR, and had the inconvenience pointed by @simoneliuzzo that parametrising a single item of an array attribute needed a special construct, set_parameter, instead of simple assignment,
3. the present variables_and_parameters branch which removes this problem by replacing all numpy arrays by ParamArrays, whether they contain parameters or not.

All the tests are run on a lattice without parameters. I display all the results in terms of ratio compared to the master branch.

linopt6

I first tried to reproduce @T-Nicholls's results by measuring linopt6 on a test lattice:

	master	variables_observables	variables_and_parameters
refpts=None, get_chrom=False	1	2.51	3.01
refpts=None, get_chrom=True	1	2.37	3.06
refpts=All, get_chrom=False	1	1.54	1.71
refpts=All, get_chrom=True	1	1.88	2.14

Ratio between 1.54 and 3.06, similar to @T-Nicholls's results. The last modification (array attributes) is indeed worse, but not so much. Remark: refpts=All is better because it's just additional python computations, without accessing lattice elements.

Tracking

The I tried a pure tracking: 10000 turns with a non-zero test particle:

	master	variables_observables	variables_and_parameters
tracking 10000 turns	1	0.97	1.01

As expected, no effect on tracking because the attribute values are initially stored in C structures, so the overhead with 10000 turns is negligible.

Attribute access

And finally I just measured a simple access to an attribute, scalar or array:

	master	variables_observables	variables_and_parameters
a = qp.Length	1	19.5	18.35
a = qp.PolynomB[1]	1	7.84	52.9

On the 1st version, array access is better probably because indexing PolynomB itself takes time both in master and variables_observables. On the 2nd step, array access is very much degraded, that's expected.

Conclusion

• concerning the last modification: as expected, it increases a lot the access time to array attributes. But on the average (linopt6), it's a 20% increase with respect to the previous version, we can discuss whether the comfort of assigning directly an item of an array rather that using a set_parameter construct is worth this penalty
• concerning the basic problem of a parametrising a lattice, the problem is in these 6 lines:

  def __getattribute__(self, key):
      attr = super(Element, self).__getattribute__(key)
      if isinstance(attr, (ParamBase, ParamArray)):
          return attr.value
      else:
          return attr

  To have a fully transparent access to parameters, both in python and C, we are adding for each access (parameter or not) a test (that should be fast), but a insistance call. Is there any better way to do that?

[simoneliuzzo]

Dear @lfarv @T-Nicholls @swhite2401,

To get more user friendly matching we lose generally a factor 1.5-3 in speed (for an already slow matching). This makes it globally less user friendly than before.

May be when GPU tracking is available we can think about it again, but as it is I would not want to use it.

Could we add the timing tests to the global tests of AT? if slower than before --> give a warning/error?

best regards Simone

[lfarv]

@simoneliuzzo: I'm not sure I understand you comment: The slowness problem comes from the introduction of Parameters, whether you use them or not. Variables are not concerned.

I understand that you propose to postpone the introduction of Parameters (until we find a better way to implement them, if is is possible), and keep only Variables? That's a good solution to go on without affecting the performance! If there is a global agreement for this, I can prepare it.

Adding timing tests in the test sequence ? Good idea. How could it be done?

• comparing the times with absolute reference values? Impossible: we don't control the "runner" machines executing the tests. Timings may vary a lot for the same code without notice,
• comparing 2 branches? That's what we do manually. GitHub tests run on the branch where the commits are pushed. I don't know if we can have access to other branches in the same test job. Anyone to look at that?

[lfarv]

For info: apparently the problem is for a large part in python. The simple fact of adding a __getattribute__ magic function doing nothing:

def __getattribute__(self, key):
   return super(Element, self).__getattribute__(key)

slows down significantly the access to the attributes:

	master	with getattribute
a = qp.Length	1	8.8
a = qp.PolynomB[1]	1	3.85

Tested with python 3.9 on macOS. With that, there is little hope that we can find a "transparent" solution for introducing parameters…

[T-Nicholls]

@lfarv, apologies for the delay in getting back to you, I was off most of last week.

Re: timing tests, I don't see why it wouldn't be possible to create a new GitHub workflow to run tests on the current branch and master, comparing how long they take. I can add this to my todo list if no-one else is keen to do it, but it'd be low priority, i.e., it'd be several months before I got around to it.

As for our slowdown, it is to be expected that overriding a default Python method with a new custom method would be slower, after all, most of the default operations are optimised "under the hood"; but I must admit that I was surprised by the magnitude of the slowdown though. As you note, this fact combined with the sheer number of getattr calls by linopt makes it inevitable that making anything that linopt needs to access a Parameter will cause a significant slowdown.

What are the primary use cases for Parameters, are they intended to be used mainly when optimising a lattice or elsewhere too? If they are only for optimisation, then I could see us having separate Parameterised element classes as a solution.

[swhite2401]

Dear all, this slow-down is clearly not acceptable and this feature cannot be merged as is. I would then suggest to separate the Parameters from Variables so we can proceed with the review of the valid part and move to Observable, matching, etc...

Concerning Parameters specifically, I think it is necessary to evaluate it in a separate branch. To honest I am also surprised by this huge slow-down but it seems we cannot do much about it with the present implementation.

My feeling is that a workaround can be found using __getattr()__ magic function instead of __get_attribute()__. __getattr()__ is a fallback solution in case the normal method do not work so in principle we could implement something that would be called only when needed, for example:

1- When parametrizing and attribute, using setattr() or other methods change its name to AttrName_Parametrized (AttrName is copied to AttrName_Parametrized and deleted) 2- In this case normal attribute access to AttrName will fail and code will fall back to __getattr()__ 3- implement __getattr()__ such that AttrName_Parametrized is also searched for 4- if AttrName_Parametrized is not found raise AttributeError

In this case for standard attributes the efficient python attribute access will be used and only in the case where a parametrized attribute is declared the slow access will be used. This should considerably speed-up methods like linopt and not affect lattices without parametrized attributes (99% of the usage)

What do you think?

[lfarv]

Hello all,

To honest I am also surprised by this huge slow-down

Yes, and it suggests that there may be some potential improvement in linopt6. @T-Nicholls already mentioned that.

For now:

1. As @swhite2401 suggest, I'll take the Parameters out of this PR. It will allow to go on. It will take some time to do that, but at least the Variable base class is prepared to accept Parameters if/when they are better implemented.
2. The idea of using __getattr__ should be tried. It will further slow down access to parameters, but on the other hand it will completely remove the penalty for "normal" attributes. In the end, it should be beneficial. For info, I tried replacing super().__getattribute(key) with object.__getattribute__(self, key). This eliminates the search across all the base classes:

   def __getattribute__(self, key):
     return object.__getattribute__(self, key)

   The single attribute access is more than twice faster ! But globally on linopt6, it's still too penalising.

[T-Nicholls]

@swhite2401's __getattr__ idea is clever and should solve the core issue if carefully implemented; we need to be very careful when creating/deleting Parameterised attributes (Parameterised and normal), that both types never exist at the same time on the same element for the same root name, e.g, elem.PolynomB and elem.PolynomB_Parameterised. Hopefully, @lfarv's nice __getattribute__ call method speed improvement can help offset the slowdown for those using Parameters.

[lfarv]

This PR now concerns only Variables, and Parameters have been removed. The title and description have been updated.

The documentation is here.

I recall that to use the new classes, one must specify:

from at.future import ElementVariable, RefptsVariable, CustomVariable, VariableList

to avoid conflicts with existing classes.

[swhite2401]

Hello @lfarv thanks for this! I have been quite busy with other stuff but this looks very nice at first sight and the documentation seems also very good. I have a small comment to start with, it concerns the class naming. Shouldn't we set Variable as BaseVariable and CustomVariable as Variable. No strong opinion just a question of taste for me.

No I have to find time for testing...

Other question/comment: did you set a branch for the Parameters? Is there some implementation we could use to test ideas to solve this slow down issue?

[lfarv]

did you set a branch for the Parameters?

The parameters are still available in the base_parameters branch. The corresponding documentation is here.

[swhite2401]

I have tested these features and overall it works very well.

Here a first round of comments:

• It would nice to have a method Element/RefptsVariables.refpts_uint32 and Element/RefptsVariables.refpts_bool that would return the refpts/mask of the elements concerned.
• How deep is the history buffer, it seems to be that it can grow infinitely -> this could be an issue for large match jobs. Can we instead set an attribut initial_value and a history_buffer with a given length. This would seem safer to me
• it would be useful to verify that the bounds are valid at initialization: that lb < value < hb is fulfilled, for now it seems to be done only at the first call of set which is ambiguous
• the example for custom variables requires that the ring is defined, this is certainly not a requirement and should not appear as such
• passing kwargs to the set/get function should be allowed

I might have more with further testing

[swhite2401]

More comments:

• I am not really keen on the syntax ElementVariable(ring[refpts], ....). I would very much prefer ElementVariable(ring, refpts, ....)
• I am not convinced at all by the relevance of ElementVariable. From personal experience I don't see much practical applications where I would favor it w.r.t to RefptsVariable. ElementVariable may even be considered unsafe providing so many functions in AT can copy elements
• Didn't we agree that these developments would go in a separate sub-package latticetools?
• set_initial would be better called reset()

[swhite2401]

Some questions:

• It could be interesting to associated variables with the Lattice_object and be able to save then. This would allow to define standard knobs that can be re-used later on. Do you think this is possible?
• One application is to compute response matrices: are these object pickable, did you check that they can be used with python multiprocessing?

[lfarv]

@swhite2401 : thanks for testing and giving comments:

It would nice to have a method Element/RefptsVariables.refpts_uint32 and Element/RefptsVariables.refpts_bool that would return the refpts/mask of the elements concerned.

I'll add such a method to RefptsVariable. But ElementVariable deals with elements, independently of the lattice, or lattices, they belong to. There is no notion neither of "lattice" not of "refpts" there (see one of you next comments…).

Can we instead set an attribut initial_value and a history_buffer with a given length. This would seem safer to me.

Ok, I can set a circular buffer for the history.

it would be useful to verify that the bounds are valid at initialization: that lb < value < hb is fulfilled, for now it seems to be done only at the first call of set which is ambiguous.

Ok, it should be easy (I did not check)

the example for custom variables requires that the ring is defined, this is certainly not a requirement and should not appear as such.

This is not a requirement (look at the class definition). In the example, ring is part of the optional *args arguments transmitted to the set/get functions

passing kwargs to the set/get function should be allowed

**kwargs arguments of the constructor are already transmitted to the set/get functions

I am not really keen on the syntax ElementVariable(ring[refpts], ....). I would very much prefer ElementVariable(ring, refpts, ....)

I am not convinced at all by the relevance of ElementVariable. From personal experience I don't see much practical applications where I would favor it w.r.t to RefptsVariable. ElementVariable may even be considered unsafe providing so many functions in AT can copy elements

Continuation of the 1st item: it really depends on your coding style. As I said before, ElementVariable ignores the lattice. The use depends on the way you build you lattices: in case the same element is repeated several times, you can either use several identical copies along the lattice, or refer to the same element several times. In the first case you will rather use RefptsVariable. In the second case ElementVariable is more efficient. Elements are stored in a set, so that even if at creation you mention an element several times, it will be stored as a single one. Personally, I try to avoid copies as much as possible, and I find ElementVariable more convenient and efficient. python is based on the use of references! And everywhere a copy argument is available, it defaults to False.

But you are not obliged to use ElementVariable if you don't like it! I understand you concern, and the help states this, both in the guide and in the ElementVariable class help:

_"An ElementVariable is associated to an element object, and acts on all occurrences of this object. But it will not affect any copy, neither shallow nor deep, of the original object"_

Didn't we agree that these developments would go in a separate sub-package latticetools?

It cannot be done for a problem of circular references, with the possibility of implementing parameters, hopefully. Because parameters need to processed in elements.py (setattr/getattr), they must be imported before elements.py. And parameters need to inherit from a common base as variables. So we need at least some common base class, whatever it is called, in the lattice package, before elements.py. But true, elements_variables.py could be moved to latticetools. I left it in lattice for proximity with the base class, but if you prefer, I can move it to latticetools.

set_initial would be better called reset()

OK

[swhite2401]

@swhite2401 : thanks for testing and giving comments:

It would nice to have a method Element/RefptsVariables.refpts_uint32 and Element/RefptsVariables.refpts_bool that would return the refpts/mask of the elements concerned.

I'll add such a method to RefptsVariable. But ElementVariable deals with elements, independently of the lattice, or lattices, they belong to. There is no notion neither of "lattice" not of "refpts" there (see one of you next comments…).

Ok to have it just in RefptsVariable

Can we instead set an attribut initial_value and a history_buffer with a given length. This would seem safer to me.

Ok, I can set a circular buffer for the history.

Here it would still be interesting to optional have an "all" option

it would be useful to verify that the bounds are valid at initialization: that lb < value < hb is fulfilled, for now it seems to be done only at the first call of set which is ambiguous.

Ok, it should be easy (I did not check)

the example for custom variables requires that the ring is defined, this is certainly not a requirement and should not appear as such.

This is not a requirement (look at the class definition). In the example, ring is part of the optional *args arguments transmitted to the set/get functions

Yes I agree, but still if ring is None it raises an error, so in reality it is required. I would add an example with and element or an arbitrary object as input for instance to clearly show a CustomVariable can connet to anything

passing kwargs to the set/get function should be allowed

**kwargs arguments of the constructor are already transmitted to the set/get functions

Agreed. But in the past I have found very useful that the get and set functions can have different kwargs

I am not really keen on the syntax ElementVariable(ring[refpts], ....). I would very much prefer ElementVariable(ring, refpts, ....) I am not convinced at all by the relevance of ElementVariable. From personal experience I don't see much practical applications where I would favor it w.r.t to RefptsVariable. ElementVariable may even be considered unsafe providing so many functions in AT can copy elements

Continuation of the 1st item: it really depends on your coding style. As I said before, ElementVariable ignores the lattice. The use depends on the way you build you lattices: in case the same element is repeated several times, you can either use several identical copies along the lattice, or refer to the same element several times. In the first case you will rather use RefptsVariable. In the second case ElementVariable is more efficient. Elements are stored in a set, so that even if at creation you mention an element several times, it will be stored as a single one. Personally, I try to avoid copies as much as possible, and I find ElementVariable more convenient and efficient. python is based on the use of references! And everywhere a copy argument is available, it defaults to False.

Sure but as soon as your save to file the referencing is gone so in effect you built a code that can use only sparsely (almost never in practice). In any case now that the code is there we can keep it. For some list manipulation the copy argument is not False by default... but it is rare I admit

I would still prefer that you change the the syntax of RefptsVariable(ring[refpts], ...) to RefptsVariable(ring, refpts, ...) this looks much more logical to me

But you are not obliged to use ElementVariable if you don't like it! I understand you concern, and the help states this, both in the guide and in the ElementVariable class help:

_"An ElementVariable is associated to an element object, and acts on all occurrences of this object. But it will not affect any copy, neither shallow nor deep, of the original object"_

Agreed, see comments above, but here we assume people users know python and read the help carefully, which is clearly not always the case (see the many issues and discussions on the topic). I can guaranty you that this will lead to comments and questions because users will not understand the behavior of the code

Didn't we agree that these developments would go in a separate sub-package latticetools?

It cannot be done for a problem of circular references, with the possibility of implementing parameters, hopefully. Because parameters need to processed in elements.py (setattr/getattr), they must be imported before elements.py. And parameters need to inherit from a common base as variables. So we need at least some common base class, whatever it is called, in the lattice package, before elements.py. But true, elements_variables.py could be moved to latticetools. I left it in lattice for proximity with the base class, but if you prefer, I can move it to latticetools.

Ok but then the naming should be improve because now we have one module element_variable.py and another varaiable_element.py: can't we put everything in variables.py at least?

set_initial would be better called reset()

OK

[lfarv]

@swhite2401 : started to implement the modifications. Here:

It would nice to have a method Element/RefptsVariables.refpts_uint32 and Element/RefptsVariables.refpts_bool that would return the refpts/mask of the elements concerned.

For a RefptsVariable, the refpts (in its original form) is available as the var.refpts attribute. So to get what you ask for, without modification, you would call:

boolrefs = ring.get_bool_index(var.refpts)

With a new RefptsVariable method, it will become:

boolrefs = var.get_bool_index(ring)

Is it worth an additional method? No problem if you find it more convenient…

[swhite2401]

Is it worth an additional method? No problem if you find it more convenient…

No you are correct it is probably not needed in fact... let's leave like it is for the moment

[lfarv]

passing kwargs to the set/get function should be allowed

*kwargs arguments of the constructor are already transmitted to the set/get functions

Agreed. But in the past I have found very useful that the get and set functions can have different kwargs

Then you put all kwargs together and each of set/get will take the one it needs. Look at my example: the set function uses ref1, ref2, and total_length, while the get function only uses ref1. This looks simpler to me than defining two sets, which very likely overlap.

[lfarv]

Ok but then the naming should be improve because now we have one module element_variable.py and another varaiable_element.py: can't we put everything in variables.py at least?

Again the same circular import problem: variables.py must be imported beforeelements.py, but element_variables.pymust be import after elements.py andlattice_object.py…

But I agree, the module name should avoid confusion with variable_elements. Any suggestion ?

[swhite2401]

Maybe lattice_variables.py?

[lfarv]

1st round of modifications:

• The abstract base class is renamed from Variable to VariableBase
• the set_initial() method is renamed as reset()
• a new input keyword, and its corresponding attribute history_length, set the maximum length of the history buffer, The default None means infinite length,
• the element_variables.py module is renamed as lattice_variables.py

[lfarv]

With the last modifications, I have two questions, more or less linked together:

1. Initial value

The initial value is no more the 1st value of the history buffer, it's independent. For some kinds of variables (RefptsVariable for instance), the value is not known at creation of the variable: it has to be read for a given ring. So the initial value is initially set to None (it could be NaN). There are two ways of initialising it:

1. Require an explicit .get(ring=ring, initial=True). That's the "logical" solution: no initial value until it is explicitly defined. But may be not the most convenient.
2. Silently fill it with the 1st get or set value. That's what was happening when the 1st history value was considered as the initial value. That's what is presently implemented, and it does not prevent from explicitly defining the initial value.

What do you think?

2. Value check versus bounds.

At the moment, the value is checked within the set method. There is no check in the get method. And I don't think there should be. But then, the test at variable creation is a problem: the variable should be read at creation (why not), but for RefptsVariable, one needs to provide a ring argument to the constructor. I don't like that because it could suggest that the variable is linked to this ring, which is not true: the actual ring is specified at set of get time.

Both questions can be summarised as: Do we force a "read" of the variable during creation ?

• Then it should be done in the base class, then this base class needs a ring=None keyword argument, which can be ignored if the variable kind does not rely on any ring. And same for the CustomVariable class, in case the user-provided get function needs this ring. No big problem there, it'almost transparent.

• And the RefptsVariable (and all derivatives of the base class working similarly) must have a ring argument, with the drawback that I mentioned above… More problematic.

[swhite2401]

For the initial value, I would stick with option 2. In general, I am not so much in favor to force a read during initialization.

For the value check let me give you a bit more details why I asked for this:

The main issue that I had with the bounds is that sometimes you wrongly set your bounds for a matching job and the initial value is outside the valid range, this results in an error 'x0 is undefined' . This is a least square minimization error but could have several causes so it was sometimes painful to diagnose and is not explicit.

I believe the x0 is calculated with the get() function in the matching because it starts by computing the initial residuals, so as you mentioned we could add the check in the get() function to prevent this...but I agree that it is not very nice... Another option would be to implement the test in the matching function but I had the same issue with other optimizers for which I also use the matching variables because they are very convenient...so it solves the issue only partially

Other than that I would have to think about it a bit more... any better idea?

[lfarv]

@swhite2401: here is a new proposal:

The variable constructor will attempt to read the variable value:

• if it succeeds, the initial value will be set, and eventually a bound check will be done (see later). This will happen for all variables not relying on a specific ring (ElementVariable for instance), or if a RefptsVariable constructor is provided a ring=some_ring keyword.
• if it fails, the initial value will be set to NaN, and no test, as for RefptsVariable created with default arguments.

ring does not appear as a mandatory argument for RefptsVariable, avoiding my previous concern. When defining variables for matching, for instance, you don't need to care about the initial value because the matching will explicitly define it before running, so you can forget the optional ring keyword. So it's basically transparent, it just offers the possibility to controlling the initial value.

Concerning the test, I'm not sure to understand your fear: if there is no check, neither at creation nor when getting the value, x0 will always be defined, possibly with out-of-bounds values. If the bounds are wrong, you'll probably get a "set value must be in {self.bounds}" error immediately when matching starts varying the parameters, but this error may also appear with correct bounds if the target needs it. In any case you must expect this kind of error! The message could be modified as: "Tried to set {self.name} out of {self.bounds}" to be more clear.

So I would keep the test where it is now: only in the set() method ann not at creation.

[swhite2401]

@lfarv 'x0 not defined' is an internal error to numpy.least_square, if the initial values are not within bound it returns this errors following an internal check I suppose. As I said we could catch this error in the matching function if it is found more convenient but I have faced the same issue using external optimization packages that can take bounds as input.

[lfarv]

New behaviour to address the questions of initial value and bound checking:

Initial value

The variable constructor attempts to read the variable value:

• if it succeeds, the initial value will be set. This will happen for all variables not relying on a specific ring (ElementVariable for instance), or if a RefptsVariable constructor is given a ring=some_ring keyword.
• if it fails, the initial value is set to NaN until the first set or get defines it.

On top, a good practice is to explicitly reset the initial values (get(..., initial=True)) before any sequence of variation, like matching,

Bound check

A new check_bounds keyword is added to the get method. The default is False, but if it it set to True, an exception will be raised if any variable is out of bounds.

The matching now starts with a line:

vini = variables.get(ring=ring, initial=True, check_bounds=True)

This resets the initial value of all variables and errors if any variable is initially wrong. This should be enough to identify a wrong starting point.

[lfarv]

And a new question:

Some properties (initial_value, last_value, previous_value,...) raise an exception if the value is not available yet. Another option is to return NaN instead of raising the exception, and let the client decide how he wants to handle it.

I have no favourite solution. What is your advice?

[MJGaughran]

Are you expecting people to use these Variable properties where performance matters, or are they typically for e.g. interactive sessions? Are these property exceptions likely to get triggered?

I think starting with exceptions is sensible for Python error handling.

Maintaining consistent error handling with the rest of pyAT is also important.

[lfarv]

Are you expecting people to use these Variable properties where performance matters, or are they typically for e.g. interactive sessions? Are these property exceptions likely to get triggered?

initial_value and last_value should not raise exceptions if the initial value is property defined by the user (implicitly or explicitly). But an error on previous_value may occur on a trial to access it before the first variation step.

I think starting with exceptions is sensible for Python error handling.

So it's one vote for "exception", which is what is presently implemented. Any other comment?

[swhite2401]

Any other comment?

An exception is fine for me

[lfarv]

This version is now stable and ready for merging. Please comment and discuss. Together with #738, it's the basis for future developments. I recall that for the moment, and to avoid conflicts, the use of Variables needs:

from at.future import RefptsVariable, ElementVariable, VariableList, ...

These will become standard in the next major version.

[swhite2401]

Hello @lfarv , thanks a lot for all these very nice developments. Unfortunately I have been swamped by many other things lately and I have not been able to look into these things. More importantly, I don't know when I will have time in the near future so I am not quite sure how to move forward...are you blocked in some way in yours developments?

[swhite2401]

@lfarv,

Looking back into this I can see a couple comments that were not implemented:

• I am not really keen on the syntax ElementVariable(ring[refpts], ....). This comment was ignore, I maintain that ElementVariable(ring, refpts, ....) is more "natural". You may allow for both syntaxes by looking for the type of the first element: if it is a lattice_object look for refpts in kwargs
• the example for custom variables requires that the ring is defined, this is certainly not a requirement and should not appear as such. This was not changed in the jupyter notebook, I think the example provided is misleading. At least an additional example not requiring a ring should be provided

[swhite2401]

I also raised the question whether we could add variables directly to the lattice object to define knobs that could then be saved in the lattice file and re-used, have you thought about this?

[lfarv]

Hello @swhite2401: about your 1st remark:

 I am not really keen on the syntax ElementVariable(ring[refpts], ....). This comment was ignore, I maintain that ElementVariable(ring, refpts, ....) is more "natural". You may allow for both syntaxes by looking for the type of the first element: if it is a lattice_object look for refpts in kwargs

I think there is still a misunderstanding about the use of ElementVariables, due to a problem in the documentation. Instead, look at this example:

import at
from at.future import ElementVariable
qf1 = at.Quadrupole("qf1", 0.5, 0.75)
v1 = ElementVariable(qf1, "PolynomB", index=1)

v1 is linked to the strength of qf1, no refpts is involved here

cell = at.Lattice([dr1, qf1, dr1, qd1,...], ...)
arc = cell + reversed(cell)

v1 now acts on both cell and arc,

ring = arc.repeat(4, copy_elements=False)
ring.insert(0, at.RFCavity( ...))

v1 now also acts on ring. The ElementVariable acts on an element wherever it appears: the notion of refpts is irrelevant. A match on cell will react on ring. You can also modify a lattice, insert elements without breaking the variable. But you cannot make copies.

A RefptsVariable has the opposite behaviour: once it's defined, you cannot modify the lattice. But you can use it on any lattice with the same elements.

To avoid confusion, I still insist that the argument of an ElementVariable must be one or several Element objects, and the argument of a RefptsVariable must be a RefPts object.

I will modify the documentation of ElementVariable to remove any mention of "refpts".

[lfarv]

About the 2nd remark:

the example for custom variables requires that the ring is defined, this is certainly not a requirement and should not appear as such. This was not changed in the jupyter notebook, I think the example provided is misleading. At least an additional example not requiring a ring should be provided

I think I see you point. I found it difficult to find a variable not linked to a ring. I thought for instance of a variable acting on the "DPStep" used in various optics functions, but it is not very useful. Varying the energy is again linked to a ring. Do you have any other example?

[lfarv]

@swhite: The notebook is updated with a modified description of ElementVariable not referring to any refpts, and simpler examples for custom variables.

These examples do not show all the possibilities offered by constructor arguments. Do you have ideas for other examples ?

[lfarv]

Concerning the idea of saving variables together with the lattice: it's indeed very attractive.

At 1st glance, simply adding the variable as a lattice attribute should allow it to be saved automatically:

v1 = RefptsAttribute("QF1", "PolynomB", index=1)
ring.v1 = v1
ring.save("ring.mat")

However this may work or not, depending on the variable type and file format:

• a CustomVariable will fail to save user-defined python code
• saving in ".m" format will fail to find a text representation of the variable.
• Even if it may work for some variables, the ".mat" format, useful for exchange with Matlab does not look well adapted for that.

The "pickle" format is more powerful and should be able to care about everything, but this is still to be checked.

So I would prefer to set this idea as the topic for another PR, so that we can have time to carefully think about it. It should not interfere with the definition of variables as it is fixed here.

[swhite2401]

My opinion is that this ElementVariable will hardly be used and is just confusing the user. We will for sure have negative feedback on this from users that do not understand its behavior (remember, we already get a lot a question regarding the basic behavior of python pointers... and now you want to build another layer on top of that....).

Anyway, if you insist on having this feature ok for me.

[swhite2401]

Being able to save these variable with the lattice is essential for the lattice designer, why not make the development now?

[lfarv]

@swhite2401:

Being able to save these variable with the lattice is essential for the lattice designer, why not make the development now?

Ok, I can start looking at that as soon as this PR is merged, which will unlock other branches: maintaining several active branches starts being difficult since elements.py and lattice_objects.py are involved in several places…

[swhite2401]

Ok then we can merge, this is anyway in the "future" imports so we can make some modification even after the merge right?

[lfarv]

this is anyway in the "future" imports so we can make some modification even after the merge right?

You are right. There may modifications resulting from reviewing the upcoming "new matching" PR which is closely linked to the "Variables" and "Observables" branches.

Can you have a look at #738, while I'm preparing the "matching" one.

[swhite2401]

Ok I will do, are you still looking at your ESRF mail? I asked you a question some times ago...