Add two-photon continuum

[jwreep]

Fixes #43

Not functional yet -- just introducing a placeholder for now.

A few questions have popped up:

1) We need the rest wavelength of the 2S1/2 (hydrogenic) and 1s2s S0 (helium-like) states. Should we use the theoretical or observed wavelength? (i.e. self._elvlc['E_th'] or self._elvlc['E_obs'])

2) Where and why does A_sum enter the calculation? It's not in the Equations in Chianti papers so far as I can see.

3) Should the output units be u.erg * u.cm**3 / u.s / u.angstrom as with f-f and f-b?

[jwreep]

Ohhh, that makes a lot of sense. Thanks for the clarification!

One query (!) -- should it be $P{2p} = \frac{d\epsilon}{d\lambda}\frac{4\pi}{n{e}n_{H}}$? In which case, the p/e ratio can be removed.

That is, instead of $\frac{1}{n{e}}$ at the end there, we have $\frac{1}{n{H}} = \frac{n{e}}{n{H}n_{e}}$ so the p/e ratio would cancel.

I guess we should also drop the abundance and ionization fraction components as well.

Oh, I see. So this is in the IonCollection? What's the reasoning? It's an easy fix in any case.

[wtbarnes]

One query (!) -- should it be P2p=dϵdλ4πnenH? In which case, the p/e ratio can be removed.

That is, instead of 1ne at the end there, we have 1nH=nenHne so the p/e ratio would cancel.

Probably? My only concern is that if we do that here, we should make sure that's the case everywhere. In the case of the line emission contribution function, this is just a matter of resolving #236. In the case of the free-free and free-bound continua, I'm actually not sure where it would come in, if at all. We would then need to make it super clear in the documentation that the expectation for any emission measure is that $EM=n_en_Hdh$.

Oh, I see. So this is in the IonCollection?

Yes, it gets added when you compute the combined continuum from multiple ions. https://github.com/wtbarnes/fiasco/blob/c015fe1902c84a23949b1e10aa855611c9aa7c86/fiasco/collections.py#L121

What's the reasoning? It's an easy fix in any case.

That's a fair question. I was trying to remind myself why earlier. I think I did this so you could calculate the continuum emission in an abundance-free manner which is useful, e.g. if you're letting the abundance be a free parameter when fitting a spectra.

[jwreep]

Right, thanks. I've done some tweaking to get this into a more consistent form as f-f and f-b. It seems to be running faster now (one less outer product).

It looks like the best agreement with IDL is with letting $EM{c} = n{e}n_{H}dh$ and eliminating the p/e ratio. In general, that would be my preference.

The calculation looks fine at some temperatures, but there might still be an outstanding issue at other temperatures:

Also, I don't understand why the tests are now failing? I haven't tinkered with anything that seems like it's related. Maybe recent asdf-astropy updates? Looks like they've migrated to asdf version 1.6.0. msg = "File 'file:///home/runner/work/fiasco/fiasco/fiasco/tests/idl/data/ioneq_26_20_v8.0.7.asdf' was created with extensio...I 'asdf://astropy.org/core/extensions/core-1.5.0' (from package asdf-astropy==0.5.0), which is not currently installed"

[wtbarnes]

It looks like the best agreement with IDL is with letting EMc=nenHdh and eliminating the p/e ratio. In general, that would be my preference.

Ok, I think I'm on the same page as well. I'll put in a PR to modify #236 so we can have these go in at roughly the same time.

The calculation looks fine at some temperatures, but there might still be an outstanding issue at other temperatures:

Well that's certainly odd...could you try calculating it over a wide range of temperatures (e.g. 1e5 to 1e8) and then plotting those as two dimensional plots and comparing those to the equivalent result from IDL? That might give us some idea of at what temperature things are breaking down. Ultimately, I think that's the test we want to run when comparing against the IDL version anyway.

Also, I don't understand why the tests are now failing? I haven't tinkered with anything that seems like it's related. Maybe recent asdf-astropy updates? Looks like they've migrated to asdf version 1.6.0.

This is an asdf-astropy thing. I'm not sure whether its a bug or not, but #272 should fix this and once that is merged, you can update your branch and the tests will run again.

[jwreep]

It's not as bad as I thought. I'm wondering if that's just a low intensity down in the numerical noise.

Edit: it looks like there are only discrepancies below 1e-40 or so. IDL's float precision is 32-bit, so only good for +-1e38 precision. It looks like the IDL has the output in double precision, but that seems suspicious. http://www.idlcoyote.com/math_tips/double.html

https://github.com/wtbarnes/fiasco/assets/33359703/f38cbe99-7606-47f5-b2fa-4dc3e5daddba

https://github.com/wtbarnes/fiasco/assets/33359703/36bf2e73-4b25-4a6b-93fb-8cffec4908c0

https://github.com/wtbarnes/fiasco/assets/33359703/99fc3def-a695-4157-9e49-5431f9b7599f

[jwreep]

I still haven't had any great ideas concerning the differences at low intensities, beyond that it might be numerical precision. I'm not sure how to proceed there. There's also still the small bump at 304, which might suggest something else outstanding.

I've added tests now (which still look like they need a bit of debugging), but I'd like to request that you perhaps could write the IDL comparison test since I'm still a bit shaky about using hissw and whether it's working properly with CHIANTI for me. The comparison plots were made by calculating directly and loading a sav file into Jupyter. Happy to provide the save files or the notebook.

[jwreep]

It's curious, but the tests are giving different values for on different OSes / Python versions. Not sure of the cause.

[jwreep]

The tests were failing with extremely small intensities in the two photon output ~ 1e-70 or so, giving different values for different OS and Python versions. I've changed it to use temperatures that give intensities of the order ~ 1e-30, and the tests are now passing. That suggests that the issue was due to roundoff errors, which might also be the case for the IDL-Python comparison at such low intensities. At this point, I admit I don't know enough about floating point math to say much more than that, and I know less about the differences in how IDL vs Python implements it.

In any case, barring that discrepancy, I think it's about ready to merge.

[jwreep]

Might need to recheck this floating point math. It might be a deeper issue. I've added tests to fill in the CodeCov gaps (He-like ion and one with a missing data set). The comparison for C V is failing, and gives a different value from what I get when calculating by hand. The different OSes/Python versions are giving slightly different answers as well.

This is frustrating . . .

[wtbarnes]

Sorry for being so unresponsive for several weeks!

The comparison for C V is failing, and gives a different value from what I get when calculating by hand. The different OSes/Python versions are giving slightly different answers as well.

Regarding the differences in OS/Python versions, having a cursory look through the CI, I see that the test that is failing for every OS/Python version is generating an output of 4.020540e-25 consistent out to those digits with some differences out past the sixth decimal place while the expected answer in the test is approximately 4.04e-25. The relatively small differences between the different CI runs is not as concerning to me as the difference between the expected value and what the tests on the CI are returning for all OS/Python versions.

These small differences between versions could be down to different versions of numpy and could be a result of solving the level populations the way I am. Especially for small level populations, there can be relatively large fluctuations due to numerical stability.

I also noticed that some of the tests are issuing warnings about missing proton data. Should the comparisons be run with or without the protons? My understanding of the above discussion was that we wanted to exclude the proton rates by default in the two-photon calculation.

but I'd like to request that you perhaps could write the IDL comparison test since I'm still a bit shaky about using hissw and whether it's working properly with CHIANTI for me.

Yes, I'd be happy to do that! Once we get the above issues ironed out, I can either push directly to this PR, or we can open up a new PR to do it.

Thanks for your persistence. This has been much more of a slog than I anticipated though to put it in perspective, it took me years (!) to sort out what the issues were with the free-free and free-bound continua!

[jwreep]

Thanks, Will! I assumed you were busy with eclipse meetings.

Yes, this is definitely a lot more of a slog than I wanted it to be. Nearing the end point, though, I hope!

while the expected answer in the test is approximately 4.04e-25.

Yep, this is what I'm getting when doing it by hand:

>>> temperature = np.logspace(5, 8, 100)*u.K
>>> c5 = fiasco.Ion('C V', temperature,hdf5_dbase_root=fiasco.defaults['hdf5_dbase_root'])
>>> c5_emission = c5.two_photon(200 * u.Angstrom, electron_density = 1e10* u.cm**(-3))
>>> c5_emission[0,30,0]
<Quantity 4.04634243e-25 cm3 erg / (Angstrom s)>

Is there anything in the smaller test database versus full database that might cause the difference?

Edit: yes! I rebuilt with the test database and get a different result.

>>> files = get_test_file_list()
>>> build_hdf5_dbase(fiasco.defaults['ascii_dbase_root'], fiasco.defaults['hdf5_dbase_root'],files=files)
>>> temperature = np.logspace(5, 8, 100)*u.K
>>> c5 = fiasco.Ion('C V', temperature,hdf5_dbase_root=fiasco.defaults['hdf5_dbase_root'])
>>> c5_emission = c5.two_photon(200 * u.Angstrom, electron_density = 1e10* u.cm**(-3))
>>> c5_emission[0,30,0]
<Quantity 4.02054043e-25 cm3 erg / (Angstrom s)>

I don't know whether that's a bug or expected, though!

There's no difference for C VI:

>>> c6 = fiasco.Ion('C VI', temperature,hdf5_dbase_root=fiasco.defaults['hdf5_dbase_root'])
>>> c6_emission = c6.two_photon(200 * u.Angstrom, electron_density = 1e10* u.cm**(-3))
>>> c6_emission[0,30,0]
<Quantity 8.25316887e-26 cm3 erg / (Angstrom s)>

which is the same as with the full database. The test in test_collections (H I + He II) also gives the same result.

[wtbarnes]

Well this is mysterious. And now the test for the full database fails (as expected given your earlier findings)! https://github.com/wtbarnes/fiasco/actions/runs/8655635096/job/23735165230?pr=260#step:10:178

I'm confused as to why this value should be dependent on the presence of other files in the database. My first thought was the proton rates, but C V has no psplups files so it can't be that.

[wtbarnes]

Ok I think I figured it out. The reason was due to the absence of the c_5.reclvl file in the test database. In the level populations calculation, there is optional correction of level resolved ionization and recombination that is applied if the reclvl or cilvl files are present. If it is missing, it is skipped as it is assumed there isn't enough information there to calculate that correction.

I've just pushed a commit that adds this file to the test database and I reverted your test back to the old value. The test and full database tests should now pass 🤞.

[jwreep]

Thanks! Would've taken me a week to figure that one out.

[wtbarnes]

It is extremely subtle. Also, its probably my fault for not adding a warning if the code tries to calculate the correction, but can't. The reason its excluded is because most ions don't have these files so you'd be getting warnings all the time unless you set the keyword argument to false.

[wtbarnes]

Ha well now the v9 test is failing because C V has no reclvl file in that version since the level-resolved ionization and recombination is treated differently.

I think the right way to resolve that is to just skip the two-photon test for v9, at least until we fully support v9 anyway and then figure out a better solution then. To do this, you can use the decorator as is done here: https://github.com/wtbarnes/fiasco/blob/93a85d8f1aad5081a1b6a4c2bc3add44f73bc58a/fiasco/tests/test_ion.py#L245

[wtbarnes]

Now that the tests are all passing, I'll do a review of the code later today and we can hopefully get this merged in the next day or so. I'm now inclined to just leave the IDL test to a separate PR.

Thanks again for your persistence!

[wtbarnes]

@jwreep I've just pushed a commit that does some minor refactoring of the logic in the 2p method on Ion and IonCollection. Have a look at it and let me know what you think.

If the tests pass here, I'm happy to merge. We'll need to open up an issue to track the IDL comparisons for this method first.

[jwreep]

All of these changes look reasonable and looks like cleaner code than mine. I think we should also open an issue to check / verify whether dielectronic ions impact this calculation, as discussed above. If so, they need to be properly incorporated.

[wtbarnes]

Thanks for this massive effort @jwreep!