Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 26 commits into from
Oct 17, 2024

Conversation

kslong
Copy link
Collaborator

@kslong kslong commented Oct 14, 2024

The main changes aside from documentation are:

  • Corrections to the radiative temperature calculation to allow for the spectral range of the photons that are generated.
  • A capability to create a fits file, containing the detailed spectra in each cell, as well as a table that the parameters of fits to the spectra by band. This requires one to have cfitsio installed somewhere, and is not part of make_all
  • Fixes to the writing of the disk_diag file so that it records more information, and does not sum the photons over multiple cycles.
  • Warning: there is an extra ionization mode possible, that does not really change things. This is a test for a different approach to ionization but currently does not do anything. Note also, that planned changes to LTE_tr and LTE_te modes have not been implemented at present.

kslong added 21 commits August 9, 2024 12:52
Only a place holder for a new mode
has been added at this point.
Basically, at this point matrix_populations2
is identical to matrix_populations, so this
is just a routine to be used to develop
a new ionization mode
The normal modes are unchanged but
various changes have been made in matrix_ion2
to slow down how rapidly the temperature
can change. This still is not
fundamentally different from the
previous approach.
Previously setting make_tables to yes, just
ran windsave totable on each cycle, now
the disk_diag file is also saved.

Cleaned up writing of the disk_diag file
when no photons hit disk.
Variables have been added to the
disk structure to allow for
better tracking of where
photons are emitted from the disk,
and where they hit the disk.
@jhmatthews jhmatthews self-requested a review October 14, 2024 16:23
@jhmatthews
Copy link
Collaborator

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
Copy link
Collaborator Author

kslong commented Oct 14, 2024 via email

@kslong
Copy link
Collaborator Author

kslong commented Oct 15, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator Author

kslong commented Oct 16, 2024

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 jhmatthews changed the title Xband xband: windsave2fits, changes to radiation temperature and fixes to disk_diag file. Oct 17, 2024
@jhmatthews
Copy link
Collaborator

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.

@jhmatthews jhmatthews merged commit 9a9cf31 into dev Oct 17, 2024
2 checks passed
@jhmatthews jhmatthews deleted the xband branch October 28, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants