A23 4yr diffuse

[toej93]

I believe we can merge now. All the new changes were tested.

[clark2668]

So, it looks like some conflicts need to be resolved?

Also, can we switch the meaning of Latten upper/lower bound in AraSim so that "upper bound" means "longer attenuation length at a given depth"? Otherwise I think it will be confusing long term.

[clark2668]

One other thing: I believe in the a23_4yr_diffuse branch, we agreed to leave in place the bug in the antenna gain (the double counting of the transmission coefficient, I think?), but it is fixed in the master branch. The fix was master commit aa2b30f if I' not mistaken. So in the master branch, we want that bugfix to survive, so we should avoid merging the bug-fix back in.

[toej93]

We can keep the bug-fix in place when resolving the conflicts. I'll change the meaning of Latten

[clark2668]

Thanks for changing the Latten definition! Otherwise, this seems okay to me. When you get a chance, any thoughts @mingyuanlu ?

[toej93]

@mingyuanlu , I see that in detector.cc you made some changes:

<<<<<<< a23_4yr_diffuse
inline void Detector::ReadVgainTop(string filename, Settings *settings1) {

// Read 600MHz low-pass filter for uncertainty estimate
ifstream in;
in.open("filter7pole600MHz.txt", std::ifstream::in);
double filterValue[60];
double filterFreq[60];
double tempFilter, tempFilterFreq;

if(in.good()){
for(int i=0;i<60;i++){
in >> tempFilterFreq >> tempFilter;
filterValue[i] = tempFilter;
filterFreq[i] = tempFilterFreq;
}
}
in.close();

=======
inline void Detector::ReadVgainTopSettings(string filename, Settings *settings1) {
>>>>>>> master

The ======= separates what is on the branch and what was on master. Which one do we want to keep?

[mingyuanlu]

Wait why are we merging a23_4yr_diffuse back to master? My understanding was that a23_4yr_diffuse is going to be left as a special branch that freezes the simulation used in our analysis for future reference and reproducibility purposes. This was also the reason behind us deciding that, for the bug of the extra transmission coefficient being applied twice and the bug that the incorrect gain file was used (PR #13), we were going to fix them in the master branch, but not in the a23_4yr_diffuse branch because we want to be able to reproduce the simulation files used in the analysis, which had the bugs. In other words, we were fine with the bugs in this special branch since it is what we used to generate simulation for analysis. But for future users, who presumably will start with the master branch, the bugs were fixed.

So merging the two branches here is not consistent with the above logic. The conflicts in Detector.cc arise because of that.

[clark2668]

Right, so I think I completely agree with leaving the bug in the gain files, that you fixed with PR #13 in the master, in the a23_4yr_diffuse brand only. I guess the question is: are there any other changes that we made--e.g., the n(z) systematics, the Latten systematics--that we would like to merge back to the master? If so, I think we can perform a selective merge, and then freeze this branch so it endures for reproducibility purposes.

[mingyuanlu]

Okay makes sense. In principle I could make the signal chain related changes work in the master branch as well, and have them used when USE_SIGNAL_CHAIN_LOWER_BOUND is set to 1. How urgent is this merge? Otherwise I'll revisit it in a few weeks.

Probably should be noted that the a23_4yr_diffuse branch should not be deleted after the merge, as one often assumes one can do. Because we don't plan to put the buggy (trans. coef. + gain file) code back to the master but we just want to keep it live in this special branch.

[clark2668]

I don't think this merge is particularly urgent, unless @toej93 disagrees for some reason. We should also make sure the n(z) and Latten systematics get pulled over too.

And yes, I agree, we should not delete this branch, it needs to be preserved for reproducability.