search

xpsi-group / xpsi

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

Disk #386

Closed Basdorsman closed 2 months ago

Basdorsman commented 3 months ago

Main feature is that the disk is implemented through the signal. Looks like this implementation style makes any new cythonization in disk signal integration is unnecessary (which I did in my fork), since it will be integrated anyways as part of the signal. I am not in a hurry to merge but I would be happy if the code is reviewed and get some feedback.

I have implemented the slower "split=False" num5D (only for stokes=False), just because I want to compare speeds, but using this option would otherwise never be recommended.

TestRun_AMXP.py is a relevant example where these features are showcased.

Basdorsman commented 3 months ago

Test now passes. By default the disk is turned off in the photosphere.py so it should not interfere with anyone's work. If it is turned on, it occupies the "custom" option.

thjsal commented 3 months ago

@Basdorsman Do you mean the example CustomPhotosphere.py? I do not see any changes done to the standard photosphere.py, unless you forgot to push them? Or you just mean that the standard photosphere.py has no disk, as before?

Basdorsman commented 3 months ago

Yes you're right. It's not in photosphere.py, it is in customPhotosphere.py. And I think we can leave it like that, I think it's fine to use the custom option in this flexible way, and it is not necessary to add the usage in the normal photosphere.py. Maybe that can be done later but I'm sure this is also not the final version of the disk anyways.

thjsal commented 3 months ago

I see! I think the new test example (TestRun_AMXP.py) had a bug when initializing the hot region object (it was selecting Num4D atmosphere instead of Num5D). I hope it is correct now? At least not all flux is zero anymore.

I tested the old examples too, and all they seem to work still fine.

I also made version and update and wrote something to the Changelog. Maybe you can extend/modify that if you want. One could also mention the new example script in this page (the others are at least listed there): https://github.com/xpsi-group/xpsi/blob/main/docs/source/Example_script_and_modules.rst

But otherwise, I think it could be merged.