igmhub / picca

set of tools for continuum fitting, correlation function calculation, cosmological fits...
GNU General Public License v3.0
29 stars 22 forks source link

Fix dla model #1022

Closed cramirezpe closed 1 year ago

cramirezpe commented 1 year ago

Fixes #1019 .

I used Garnett 2018 as a reference as it seemed more clear to me, in the issue the comparison with the old method is detailed.

cramirezpe commented 1 year ago

pylint fails with: py/picca/delta_extraction/masks/dla_mask.py:8:0: E0611: No name 'voigt_profile' in module 'scipy.special' (no-name-in-module)

This makes no sense.

Also I had to change lots of variable names (e, T, mp, me) to more confusing ones because pylint complained...

cramirezpe commented 1 year ago

I also added Naim because there are differences in the Lyman beta region between my code and his: compare_voigt (1)

There might be an error with the constants somewhere..

iprafols commented 1 year ago

After discussing it with @cramirezpe I added a few changes to the code to further optimize. Basically, I merged the two tau functions (they were almost identical) and computed the prefactors that were independent of the DLA parameters outside of the function so that they are only computed once

I also improved the docstring adding the exact equations we use and renamed a few of the variables

From my side this PR is ok, but I think we should wait to merge until  @p-slash has review it and the discrepancies with the Lyman Beta profiles are understood

p-slash commented 1 year ago

I also added Naim because there are differences in the Lyman beta region between my code and his: compare_voigt (1)

There might be an error with the constants somewhere..

The constants seem to be the same. Maybe we are seeing the thermal broadening. I think Lyb transition is weaker than Lya, so it will saturate at higher column densities. If this is the case, the choice of b (or T) will affect the profile. The other possibility is that we are seeing difference in approximations.

I will make a comment regarding what to use for oscillator strength and Einstein coefficients while replying to Michael.

Waelthus commented 1 year ago

I agree with @p-slash that Lyβ needs higher NHI to saturate and thus will be affected more by thermal broadening. What are we using for the b-parameter in both approaches? Are those fixed values for any DLA or something more sophisticated? How well do we know those values? And how large is the impact of the Lyβ differences? Ideally, we should have a unified approach that is used for both analysis packages and the mocks with some characterization of its uncertainties.

iprafols commented 1 year ago

Regarding the difference in the profile, @p-slash which value are you using for the speed of light? I noticed that the reason why the tests are failing is because I changed the value of the speed of light that Cesar was using to the one stored in scipy.constants. Not sure if this would be enough to explain the differences, though

p-slash commented 1 year ago

I use c=299792.458 km/s from Wikipedia

iprafols commented 1 year ago

Hi @p-slash Cesar uses 2.9979e8, and scipy.constants.speed_of_light= 299792458.0, the same as you use. Probably it's better to use the same value so that we can rule out that the differences come from here. I'll do a commit to change this here

Waelthus commented 1 year ago

Given that c=299792458 is an exact physical definition, I agree we should use that value and not some approximated one whereever possible...

iprafols commented 1 year ago

done @cramirezpe can you confirm if you are still seeing the difference you were seeing before?

cramirezpe commented 1 year ago

All three agree (picca_old, picca_delta_extraction and qsonic) Thanks! image

iprafols commented 1 year ago

then let's merge this and close the issue