igmhub / picca

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

Remove blinding civ [bump minor] #1046

Open iprafols opened 8 months ago

iprafols commented 8 months ago

Blinding is removed when running on metal forests

andreufont commented 8 months ago

Another comment: when @julienguy runs his analysis of CIV auto-correlations, he actually tells Picca to run the Lya auto-correlation but passes CIV deltas. He does this because he wants to see use the "wrong" Lya coordinates that we use in the main analysis, but without having to deal with the Lya correlation "contaminating" the CIV signal.

Similarly, we don't want to blind either the QSO x CIV(LYA) correlation (that Cesar is computing).

So we should only blind when we have all of the above TRUE:

Even when only one of these is False we should not blind. Do you agree @julienguy ?

iprafols commented 8 months ago

Hi @andreufont , I fixed the comments on the code. Thanks! Regarding your question about the changes in the test suite, since I had to add a couple of keywords in the headers, the automatic tests were complaining that the headers were not the same (thus my having to update them)

iprafols commented 8 months ago

@andreufont , are you happy with the changes? should we merge this?

iprafols commented 8 months ago

I fixed the bugs, but indeed we should fully test this (I have not had the time to properly do it)

iprafols commented 8 months ago

tests (python 3.10) are failing due to a weird coveralls error. Maybe their server is down. Otherwise, they might have updated something and the new version is not backwards compatible. We should try to rerun failing tests in a while to see if it was just their server being down or not

Waelthus commented 8 months ago

server seems to not be down according to this. Restarted the tests which was not successful, but apparently even with servers up they had similar issues half a year back due to software updates on their side, lets just wait...

andreufont commented 8 months ago

@iprafols , @Andrea-MG - I think we should change the order of the if clauses.

if tracer1 not in blinding_tracers or tracer2 not in blinding_tracers:
    # one of the tracers is not relevant, so do not blind (ever)
    blinding=None
else:
    # we are now in the danger zone, the only way out is if there is no LyA absorption in this region
    if min_rf_wave > lya_wave:
        blinding=None
Andrea-MG commented 7 months ago

The changes look good. Let's see if @Andrea-MG and @julienguy can run their CIV codes on this branch and get unblind results, before we merge.

I sent feedback to @iprafols and after the fixes I could run successfully cf, xcf, dmat and xdmat with these changes, testing with CIV and Lyb forests.