Skip to content

Conversation

@eltos
Copy link
Member

@eltos eltos commented Dec 19, 2025

Description

In recent versions, Bend.k0 may return "from_h" as a string. Getting a string where a float is expected likely breaks a lot of code, and requires users to always check for this special string to get the actual value. It's also not necessary, given that Bend.k0_from_h exists and can be used to determine if k0 is set manually or determined by h.

Not sure what the rationale behind returning a special string here was, but I propose returning the actual value (the value of h) in such cases.

Also fixes a missing assignment of k0 if h is set to zero

Related: xsuite/xplt@3158107

Checklist

Mandatory:

  • I have added tests to cover my changes
  • All the tests are passing, including my new ones
  • I described my changes in this PR description

Optional:

  • The code I wrote follows good style practices (see PEP 8 and PEP 20).
  • I have updated the docs in relation to my changes, if applicable
  • I have tested also GPU contexts

@giadarol
Copy link
Member

giadarol commented Jan 5, 2026

This was actually the implementation we had until a few weeks ago. We had to move away from it because it generates an inconsistency.

In several places, we make the assumption that the state of an element does not depend on the order in which its properties are set (e.g. when we clone elements propagating their expressions, or when building objects from dict/json). Unfortunately the implementation proposed here breaks this assumption. For example:

b1 = xt.Bend()
b1.length = 1
b1.angle = 3
b1.k0 = 3 # sets b1.k0_from_h = False
b1.k0_from_h = True
# The resulting element has h=3, k0=3, k0_from_h=True

b2 = xt.Bend()
b2.length = 1
b2.angle = 3
b2.k0_from_h = True
b2.k0 = 3 # sets b1.k0_from_h = False
# The resulting element has h=3, k0=3, k0_from_h=False

Here b1 and b2 have exactly the same properties, just set in different orders, but end up heaving different values for k0_from_h

For this reason, we need to keep b.k0 = 'from_h' when k0_from_h is True.

The value of the strength is always available in b._k0 independently of the state of k0_from_h. To avoid accessing "private" data we could implement a read-only property:

@property
def k0_value(self):
    return self._k0

We understand this kind of changes can be annoying. We are doing them now with the objective of stabilizing the API over 2026 (bear with us please :). Already today, there are no further changes foreseen for the magnets API.

@eltos
Copy link
Member Author

eltos commented Jan 7, 2026

I understand your intention and I fully agree that consistent cloning is important. However, I don't understand the example you gave, as this PR does not touch the k0 setter, and the code in the example is not affected by this PR. What you probably meant to say, is that doing b2.k0 = b1.k0 is broken if the k0 property does not return the special string, the k0 setter can act upon. If this is what you meant, I can follow your arguments.

The value of the strength is always available in b._k0 independently of the state of k0_from_h.

For this, wouldn't the fix in the length setter part of this PR be required to also handle the (rare) case of length 0 ? If so, you could at least cherry pick e46c94b

@eltos
Copy link
Member Author

eltos commented Jan 7, 2026

A possible alternative solution, if you are interested, would be the following:
If we clearly distinguish between the internal state (_k0) and user exposed properties (k0), inconsistencies can be strictly avoided and the change of issues reduced. Cloning, serialising and deserialising objects is then done using only the internal state variables (ideally with proper clone, to_dict, from_dict like methods across the whole class hierarchy with proper super calls). Users on the other hand should touch the exposed properties to alter the object state consistently. And if a user changes them, then it makes sense that the order matters (as in your example where k0 is set to an actual value), which is the present behaviour already now.

Such a refactoring will probably require a larger effort, let me know if you want to further discuss it or not.

@giadarol
Copy link
Member

giadarol commented Jan 7, 2026

I understand your intention and I fully agree that consistent cloning is important. However, I don't understand the example you gave, as this PR does not touch the k0 setter, and the code in the example is not affected by this PR. What you probably meant to say, is that doing b2.k0 = b1.k0 is broken if the k0 property does not return the special string, the k0 setter can act upon. If this is what you meant, I can follow your arguments.

The value of the strength is always available in b._k0 independently of the state of k0_from_h.

For this, wouldn't the fix in the length setter part of this PR be required to also handle the (rare) case of length 0 ? If so, you could at least cherry pick e46c94b

yes, I agree I will cherry pick that one

@giadarol
Copy link
Member

giadarol commented Jan 7, 2026

A possible alternative solution, if you are interested, would be the following: If we clearly distinguish between the internal state (_k0) and user exposed properties (k0), inconsistencies can be strictly avoided and the change of issues reduced. Cloning, serialising and deserialising objects is then done using only the internal state variables (ideally with proper clone, to_dict, from_dict like methods across the whole class hierarchy with proper super calls). Users on the other hand should touch the exposed properties to alter the object state consistently. And if a user changes them, then it makes sense that the order matters (as in your example where k0 is set to an actual value), which is the present behaviour already now.

Such a refactoring will probably require a larger effort, let me know if you want to further discuss it or not.

The issue is that when we clone we need to preserve the xdeps expressions (which in general creates a lot of constraints but are a must to handle large machines). The expressions control user-exposed properties and not internal state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants