Remove blinding civ [bump minor] #1046

iprafols · 2023-10-24T13:43:55Z

Blinding is removed when running on metal forests

bin/picca_fast_metal_dmat.py

bin/picca_fast_metal_xdmat.py

andreufont · 2023-10-24T14:00:08Z

py/picca/io.py

-    else: # This is for BinTable format
+
+        # check if we are doing metal forests
+        if "MIN_RF_WAVE" in header and "MIN_RF_WAVE" in header:


@iprafols - you probably meant MAX_RF_WAVE in one of these clauses

andreufont · 2023-10-24T14:00:53Z

py/picca/io.py

+            max_rf_wave = header["MAX_RF_WAVE"]
+            if min_rf_wave > lya_wave and max_rf_wave > lya_wave:
+                if lambda_abs2 is None:
+                    lambda_abs2 = lambda_abs


Why do you need to set lambda_abs2 here?

andreufont · 2023-10-24T14:01:42Z

py/picca/io.py

+                if lambda_abs2 is None:
+                    lambda_abs2 = lambda_abs
+                blinding_tracers = ["LYA", "QSO"]
+                if tracer1 not in blinding_tracers and tracer2 not in blinding_tracers:


@iprafols - I think you want to have an "or" here, since we don't want to blind QSO x CIV either. Basically, you want to blind only when both tracers are either LyA or QSO

@iprafols - it looks like you have fixed this above (for the ImageHDU format) but you have not fixed it for the BinTable format. It still says "and" instead of "or"

andreufont

@iprafols - I added several comments on the code, most of them are important enough that should be fixed.

I'm also surprised by the large number of files changed, in particular some log files related to picca delta attributes? Are these related to automated testing, or were they added by mistake?

andreufont · 2023-10-24T14:08:20Z

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:

lambda_rf_min < lambda_lya
tracer1 in [QSO, LYA]
tracer2 in [QSO, LYA]

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

iprafols · 2023-10-24T15:09:04Z

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 · 2023-11-06T16:28:07Z

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

andreufont · 2023-11-07T05:49:32Z

bin/picca_xcf_angl.py

@@ -253,7 +253,7 @@ def main(cmdargs):
    xcf.lambda_abs = constants.ABSORBER_IGM[args.lambda_abs]

    # read blinding keyword
-    blinding = io.read_blinding(args.in_dir)
+    blinding = io.read_blinding(args.in_dir, args.lambda_abs, args.lambda_abs_obj)


I would have expected the second argument here would be 'obj_name' as in picca_xcf, but I noticed that this variable is named different in this (not very popular) script, so we can leave it as is for now.

andreufont

@iprafols - has this been tested? I found a couple of bugs that I think should be fixed, and it would also be good if someone looking into these (Andrea? Julien? Cesar?) checked that the branch works as intended.

andreufont · 2023-11-07T06:06:52Z

py/picca/io.py

+            max_rf_wave = header["MAX_RF_WAVE"]
+            if min_rf_wave > lya_wave and max_rf_wave > lya_wave:
+                if tracer2 is None:
+                    tracer2 = lambda_abs


@iprafols - where have you defined lambda_abs? Shouldn't you use here tracer2 = tracer1?

Have you tested this code? It looks like it should have crashed...

andreufont · 2023-11-07T06:08:41Z

py/picca/io.py

+            max_rf_wave = header["MAX_RF_WAVE"]
+            if min_rf_wave > lya_wave and max_rf_wave > lya_wave:
+                if tracer2 is None:
+                    tracer2 = lambda_abs


Again, this should be tracer2 = tracer1

andreufont · 2023-11-07T06:09:35Z

py/picca/io.py

+                if lambda_abs2 is None:
+                    lambda_abs2 = lambda_abs
+                blinding_tracers = ["LYA", "QSO"]
+                if tracer1 not in blinding_tracers and tracer2 not in blinding_tracers:


@iprafols - it looks like you have fixed this above (for the ImageHDU format) but you have not fixed it for the BinTable format. It still says "and" instead of "or"

iprafols · 2023-11-07T10:00:27Z

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

andreufont

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.

iprafols · 2023-11-07T10:11:49Z

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 · 2023-11-07T13:04:58Z

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

I don't think we are doing this right. When one of the tracers is not LYA or QSO, the rest-frame range shouldn't matter. In other words, if you are looking at CIV absorption in the LYB region, you should not blind either.

andreufont · 2023-11-15T07:56:44Z

@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 · 2023-11-23T20:17:39Z

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.

Andrea-MG

I have run successfully cf, xcf, dmat and xdmat with these changes, testing with CIV and Lyb forests with @iprafols and get in all "blinding = none" in all.

andreufont · 2023-12-11T21:33:14Z

py/picca/io.py

+                if ((min_rf_wave > lya_wave and max_rf_wave > lya_wave) or
+                    (min_rf_wave < lyb_wave and max_rf_wave < lyb_wave)):
+                    # we are doing a metal forest, so do not blind (ever)
+                    blinding = "none"


@iprafols , @Andrea-MG - Are you sure about this? It looks to me that the this code would not blind the QSO x LyA(LyB) correlation.

I believe the "if" statement in line 263 should only have the first line (check that both rf_wave are above lya_wave), but it should not have the second condition.

iprafols added 3 commits October 24, 2023 15:25

removed blinding for metal forest runs

ccfa139

updated test suite

da7bc15

fixed docstring, renamed variables

b3fec96

iprafols requested review from andreufont, julienguy and cramirezpe October 24, 2023 13:43

andreufont reviewed Oct 24, 2023

View reviewed changes

bin/picca_fast_metal_dmat.py Outdated Show resolved Hide resolved

andreufont reviewed Oct 24, 2023

View reviewed changes

bin/picca_fast_metal_xdmat.py Outdated Show resolved Hide resolved

andreufont reviewed Oct 24, 2023

View reviewed changes

andreufont requested changes Oct 24, 2023

View reviewed changes

iprafols added 2 commits October 24, 2023 17:06

fixed copy/paste typos

bbf3094

fixed changes in read_blinding

4ca937c

andreufont self-requested a review November 7, 2023 05:45

andreufont reviewed Nov 7, 2023

View reviewed changes

andreufont requested changes Nov 7, 2023

View reviewed changes

fixed bugs

53e9849

andreufont reviewed Nov 7, 2023

View reviewed changes

andreufont requested a review from Andrea-MG November 7, 2023 10:02

iprafols added 3 commits November 10, 2023 21:51

fixed HDU name

adc6bcf

fixed read_blinding function

742e466

fix to coveralls workflow

eb3e58a

iprafols mentioned this pull request Nov 14, 2023

fixed the coveralls workflow #1051

Merged

iprafols added 2 commits November 15, 2023 08:28

exluded lyb forest

c939e86

excluded lyb forest

f8f1c1b

andreufont requested changes Nov 15, 2023

View reviewed changes

swaped ifs order

17742fd

Andrea-MG approved these changes Dec 7, 2023

View reviewed changes

iprafols requested a review from andreufont December 11, 2023 07:38

andreufont reviewed Dec 11, 2023

View reviewed changes

iprafols closed this Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove blinding civ [bump minor] #1046

Remove blinding civ [bump minor] #1046

iprafols commented Oct 24, 2023

andreufont Oct 24, 2023

andreufont Oct 24, 2023

andreufont Oct 24, 2023

andreufont Nov 7, 2023

andreufont left a comment •

edited

Loading

andreufont commented Oct 24, 2023 •

edited

Loading

iprafols commented Oct 24, 2023 •

edited

Loading

iprafols commented Nov 6, 2023 •

edited

Loading

andreufont Nov 7, 2023

andreufont left a comment

andreufont Nov 7, 2023

andreufont Nov 7, 2023

andreufont Nov 7, 2023

andreufont Nov 7, 2023

iprafols commented Nov 7, 2023

andreufont left a comment

iprafols commented Nov 7, 2023

Waelthus commented Nov 7, 2023 •

edited

Loading

andreufont left a comment

andreufont commented Nov 15, 2023

Andrea-MG commented Nov 23, 2023

Andrea-MG left a comment

andreufont Dec 11, 2023

Remove blinding civ [bump minor] #1046

Remove blinding civ [bump minor] #1046

Conversation

iprafols commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreufont left a comment • edited Loading

Choose a reason for hiding this comment

andreufont commented Oct 24, 2023 • edited Loading

iprafols commented Oct 24, 2023 • edited Loading

iprafols commented Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

andreufont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iprafols commented Nov 7, 2023

andreufont left a comment

Choose a reason for hiding this comment

iprafols commented Nov 7, 2023

Waelthus commented Nov 7, 2023 • edited Loading

andreufont left a comment

Choose a reason for hiding this comment

andreufont commented Nov 15, 2023

Andrea-MG commented Nov 23, 2023

Andrea-MG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreufont left a comment •

edited

Loading

andreufont commented Oct 24, 2023 •

edited

Loading

iprafols commented Oct 24, 2023 •

edited

Loading

iprafols commented Nov 6, 2023 •

edited

Loading

Waelthus commented Nov 7, 2023 •

edited

Loading