search

desy-ml / cheetah

Fast and differentiable particle accelerator optics simulation for reinforcement learning and optimisation applications.
https://cheetah-accelerator.readthedocs.io
GNU General Public License v3.0
33 stars 13 forks source link

95 nx tables import #96

Closed jank324 closed 11 months ago

jank324 commented 11 months ago

Implements reading from NX Tables, a CSV-like format used at ARES to save lattices and often considered as the ground truth there.

In this specific case this was needed to be able to read the lattice with the new geometry of the experimental area for Stage 4 v3.9, as there was no Ocelot conversion available for this.

jank324 commented 11 months ago

This is mostly done. The remaining todos are:

jank324 commented 11 months ago

The second is loaded from NX Tables.

Screenshot 2023-10-20 at 08 51 34

cr-xu commented 11 months ago

The code looks good to me. I was actually hesitating on this PR because I don't know if this feature is too problem-specific. I.e. whether

  1. they should be put at the same level as other converters which are widely used simulation codes (this might make cheetah very heavy in the future),
  2. or in some specific folders, something like special_features with example notebooks explaining the usage cases,
  3. or only in the specific problem repositories (pro: cheetah remains minimal and clean, con: we don't have reference to the potentially useful features) So that's like a design decision we'll have to make @jank324
jank324 commented 11 months ago

Hmmm ... so I did also wonder if it's so good to put ARES-specific code in here ... I actually don't know if these NX Tables are also used for other DESY accelerators.

For purely selfish reasons, I would like for this to be in Cheetah. It makes my code nicer.

While I don't think this gets in the way of other people using Cheetah, you are right that one day it might make Cheetah more bloated. This is not so much an issue with this converter being there than it is with this converter having to be maintained.

Right now, I don't think it makes Cheetah bloated. We could put it into an experimental (or similar) submodule of converters to highlight the fact that it is specific, so if it breaks in the future, it doesn't need an immediate fix. But it is also possible that in doing so, we bloat Cheetah now in an effort to avoid bloat in the future that might never come.

Difficult ... I will sleep over it.

jank324 commented 11 months ago

@cr-xu I have thought about it and I think we should keep this as is.

For now the converters are not overflowing and we have no idea how much or little they will grow in the future. I think we shouldn't solve a problem now that we don't actually know yet we will have ... and then cross that bridge if and when we come to it.