search

sirocco-rt / sirocco

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

Changes required to permit more macroatoms #1042

Closed kslong closed 11 months ago

kslong commented 11 months ago

These are the changes needed to allow CNO macro atoms to be added. From the Python perspective, these mainly relax some of the times where an error triggers and exit. This change also contains fixes to speed python up by avoiding certain gsl integrations. Updated tools for generating macro atom data are provided, but no new macro atom data is currently included

jhmatthews commented 11 months ago

This failed the CI tests in the balmer test section. This might be OK, but it means that something has changed in macro-atoms which leads to different line emissivities that are more than 30% outside the expected bound, so we might want to understand that.

jhmatthews commented 11 months ago

I should say that the test itself is not perfect, and should probably be looked at more carefully. The idea is that you set up a thin shell model in such a way that you have Case A type recombination. So I suspect that the changes to alpha_sp are causing the difference here, but I'm not certain. There's probably a better way to test the case A limit within Ed's new unit test framework. I'm wondering if the inability to reproduce the Osterbrock values may be due to lack of accuracy of alpha_sp when obtained from an integration.

kslong commented 11 months ago

I agree we need to look at this. Is the Balmer test part of the regression test?

From: James Matthews @.> Reply-To: agnwinds/python @.> Date: Wednesday, December 20, 2023 at 5:46 AM To: agnwinds/python @.> Cc: Knox Long @.>, State change @.***> Subject: Re: [agnwinds/python] Changes required to permit more macroatoms (PR #1042)

This failed the CI tests in the balmer test section. This might be OK, but it means that something has changed in macro-atoms which leads to different line emissivities that are more than 30% outside the expected bound, so we might want to understand that.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/agnwinds/python/pull/1042*issuecomment-1864337196__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!2oUh3ENPXqKT_BmhcHhrOs7MH7SLaAS6RCkIIQhvgjJUUGkaKVPIHV_h9JdSgsK8MYVoe_OtcZBbDkUSkdnBGg$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AATJ4VJ6L6AGWFX4XTRRJ3TYKLFYRAVCNFSM6AAAAABA3344J2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRUGMZTOMJZGY__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!2oUh3ENPXqKT_BmhcHhrOs7MH7SLaAS6RCkIIQhvgjJUUGkaKVPIHV_h9JdSgsK8MYVoe_OtcZBbDkW_MVgALg$. You are receiving this because you modified the open/close state.Message ID: @.***>

jhmatthews commented 10 months ago

The parameter file is not currently in the regression/ directory (partly because the idea of the test is a little different), but it is run as part of CI/gh-workflows. Note that it is possible that this failure actually caught the apparent error with the new alpha_sp routines, but that doesn't mean it's optimal.