xsuite / xobjects

Library to create and manipulate serialized objects in CPU and GPU memory and to compile code on these platforms.
https://xsuite.readthedocs.io
Apache License 2.0
4 stars 15 forks source link

Refactor parts of Xobjects, propose introduction of inheritance #125

Closed szymonlopaciuk closed 2 months ago

szymonlopaciuk commented 9 months ago

Description

This PR aims introduces:

Caveats:

Checklist

Mandatory:

Optional:

rdemaria commented 9 months ago

Thanks for this big PR. Here are some quick comments from a superficial reading:

szymonlopaciuk commented 9 months ago

Thanks for your comments! I think in the immediate future I will focus on separating the-things-we-want-now and the-things-to-be-experimented-on-in-the-future. Definitely it's also necessary for me to test the performance of this implementation vs. the old one. Sadly I've not managed to do that yet, but I will try before I come back as well. That said, some comments/answers to your comments:

Inheritance of structs it is an interesting idea, but causes Structs not using this feature to have larger size (e.g. doubling all drifts , without benefits and the derived class have slower access to members (if I have understood well). Still probably worth experimenting; therefore, I would keep the old Struct as is and add another say IStruct that implements this feature without interfering with Struct.

The slower access to members should only affects the first dynamic field, the rest of the implementation is basically the same. As for doubling drifts: this is not a problem for the "Struct" of this PR, as they remain static sized. If implemented with the XoClass in the current implementation of the Line, then they would increase in size, though it's potentially not a disaster: if we were to put the elements derived from the XoClass, we can drop the usage of UnionRef, so the grand memory total is not necessarily changing so much (esp. if a hypothetical static version existed), but the implementation of the tracker could be simplified greatly. (The big question is whether the latter is worth the effort though...)

XoClass I think I understand the idea, but I feel that using inheritance to express similar methods, mixes two concepts: data structure and methods. Why is it different from (I)Struct?

Just to separate the code and the concept of date structure and the methods. And mostly to make it easier to remove if we don't want it.

Removal of unused code: it looks like some useful code has been removed. It relates to pre-init post-init hook, which could be implemented differently, but it is needed. The methods for arrays types are needed as well, I think.

My approach was basically to run the tests without and see that nothing breaks (Xtrack), but it's not a problem to readd them.

giadarol commented 2 months ago

Stale