search

xpsi-group / xpsi

X-PSI: X-ray Pulse Simulation and Inference
Other
34 stars 20 forks source link

Atmosphere switching without re-installing X-PSI #303

Closed thjsal closed 1 year ago

thjsal commented 1 year ago

Atmosphere wrapper introduced for switching between extensions without re-installing X-PSI. The implementation is not finished for all the different integrators and atmosphere+elsewhere options, but it is ready for comments. And I tested that now I can already run both the TestRun_BB.py and TestRun_NumBeam.py examples without re-installing X-PSI.

thjsal commented 1 year ago

So the idea is to add the imports of all the atmosphere extensions (the current and any future updates) to the file called hot_wrapper.pyx (and perhaps separate elsewhere_wrapper.pyx for elsewhere), which takes an input argument to determine which atmosphere module is used when calculating. I made this new file to avoid repeating the atmosphere options in all the various integrators that are currently separately calling the atmosphere interpolations. Now in all those integrators, one has to just import hot_wrapper instead of hot, and pass an argument to the init_hot() function specifying which atmosphere module to use.

Originally, I tried also to import only the user-specified module, but that seemed not possible to do with C functions, which would all anyway get imported when compiling X-PSI, and X-PSI would just use the one that was imported last (since cannot even change the name of the functions after/while importing them). Therefore, needed to go with hard-coding the atmosphere options in X-PSI.

Basdorsman commented 1 year ago

I would suggest we try to change the integer flags to string flags? BB, NSX, BEA, ACC? I don't think you have implemented the passthrough of the argument from the integrator yet right?

thjsal commented 1 year ago

Sounds like a good idea! Need to think about the names though. I did implement the passthrough for one of the integrators (the azimuthal invariant one, which is used by the example scripts), but not yet for the rest.

thjsal commented 1 year ago

@Basdorsman Do you think string flags are enough for the user, but we could still use integer flags at the C-code level? I just updated the code to take the arguments as strings but to convert them to integers in HotRegion.py. Having integer flags at C-level seems easier for me, and perhaps also faster. However, I agree it would be then good to comment on the core level as well that which number corresponds to which model.

sguillot commented 1 year ago

Just a note the the tutorial notebook and installation instructions will need to be updated to account for this "major" change in the functionality of X-PSI.

sguillot commented 1 year ago

I have two suggestions:

thjsal commented 1 year ago

With the GSL-fix, I actually moved X-PSI from 2.0.0 to 2.0.1 (I tagged it with 2.0.1 but did not "release"), although I forgot to mention that in the final commit message. For the next upgrade, I agree we could do both this atmosphere update, and the remaining bug fixes in the likelihood calculation, and release version 2.1.0.

sguillot commented 1 year ago

@thjsal , I have another comment: I think we should give a notebook example of a CustomPhotosphere with the numerical atmosphere, similar to that in TestRun_Num.py. Previously, users could look at the Zenodo files and scripts to duplicate, but without it, I think the "preload" function of the numerical atmosphere should be in a notebook (the TestRun_Num script perhaps isn't detailed enough). Do you agree ?

thjsal commented 1 year ago

@sguillot Thanks for checking! I will do fixes based on your comments. Having a notebook version of TestRun_Num.py sounds a reasonable idea to me. But I am not sure if I understood what details you would like to add into that.

sguillot commented 1 year ago

@sguillot Thanks for checking! I will do fixes based on your comments. Having a notebook version of TestRun_Num.py sounds a reasonable idea to me. But I am not sure if I understood what details you would like to add into that. And even if the preload() function is there, it is not readily usable for a CustomPhotosphere

The tutorial notebook are more meant for that. But at the moment:

What would be useful is an additional notebook (or an added part in an existing notebook) showing how to define a CustomPhotosphere class with a generic hot_atmosphere loading method. When I say generic, I mean something with size passed as an argument (like in the preload of the surface radiation field tool notebook), rather than the hard-coded size=(..,..,..,..) that is in the TestRun_Num.py.

I see the TestRun_Num.py as a script to test everything (without really expecting the user to dig into it). The atmosphere part of X-PSI is an important one, so I think it's crucial to provide an explicit tutorial for users to learn how to switch between BB and Num4D.

Does that make sense ? I'm going to work on a quick tutorial notebook showing a pulse profile for BB and one for Num4D (for all other parameters being the same). I'll commit it here on this branch

sguillot commented 1 year ago

Hi @thjsal I made a small commits to default to BB instead of defaulting to USER in the hot_wrapper and elsewhere_wrapper. Do you agree with this ?

I am not 100% sure about it personally, for the following reasons:

Thoughts ?

thjsal commented 1 year ago

Hi @sguillot! I think what you now coded looks good to me (so apparently the second option of your list). I think making it crash might be even better (and it actually should already crash based on the checks made even before calling these functions), but as far as I remember, doing it in a clean way in Cython was not very trivial, so I would leave it as future enhancement if needed.

sguillot commented 1 year ago

Ok, so we can open an issue for future improvements, and we are good to go with this PR.