sirocco-rt / sirocco

This is the repository for Sirocco, the radiative transfer code used to winds in AGN and other syatems
GNU General Public License v3.0
27 stars 24 forks source link

xband: windsave2fits, changes to radiation temperature and fixes to disk_diag file. #1104

Closed kslong closed 2 days ago

kslong commented 5 days ago

The main changes aside from documentation are:

jhmatthews commented 5 days ago

Thanks for doing this Knox. This looks good overall, but before we merge we jsut need to

a) make it pass the tests (something failing at the moment in make check) b) I'd like to think a bit more about the T_R changing based on spectral range - something about this still doesn't sit quite right with me.

kslong commented 5 days ago

The problem is in the unit_test and does not show up on either may Mac or Linux. It’s associated with the test ionization mode I put in. I probably could remove it.

The radiation change is actually important. If you have fully covered the spectral range of the emission it does not matter, but for example if you set the low end of the ionization frequencies to 0.2 eV or something, and have low temperatures, one gets a very wrong answer to thae actual temperature.

Knox

kslong commented 4 days ago

I removed the mode I was planning to create for multicylcle determination of the temperature, since I will not have time to work on this before the transition to Sirocco. The tests got further but are now failing later in one of the tests for things that I have not touched. My guess is that something about the tests has changed, and that possibly one should to ahead. Or I can try merging dev in to xband first.

But I note there is no documentation for developers of the tests , and the reason I was intially failing is the some of the Makefiles in tests must be updated if you add a file. Documenation is needed going forward.

jhmatthews commented 3 days ago

Unit test documentation is here: https://agnwinds.readthedocs.io/en/dev/developer/tests.html

It looks to me like the reason the tests fail is because FRACTIONAL_ERROR was changed from 0.03 to 0.001 and FRACTIONAL_ERROR is used in a different test - since this would change our convergence criterion for ne, I'm assuming this is not intended?

kslong commented 3 days ago

I did change the fractional error because we were concerned about some of the ionization results, but it does not make any difference. So I have restored the old value Note that this is also defined separately in saha.c. The tests now show no errors, but they also do not seem to finish.

I apologize for missing the note about help for running the tests; neither currently works on either my Mac or Ubuntu. I will have to look into that after I return from fishing. Hopefully though this pull request can be merged, so we can restarte with Sirocco.

jhmatthews commented 2 days ago

Please note that, because I don't want to change the calculation of t_r just before release I've currently made the t_r calculation depend on a variable BAND_CORRECTED_TRAD in estimators_simple.c. Setting this to TRUE will turn on the band corrected version Knox has implemented, but this should probably either depend on ionization mode or depend on a runtime flag.