litebird / litebird_sim

Simulation tools for LiteBIRD
GNU General Public License v3.0
18 stars 13 forks source link

HWP systematics #232

Closed nraffuzz closed 9 months ago

nraffuzz commented 1 year ago

This PR has been created to address HWP systematics

ziotom78 commented 1 year ago

Hi @nraffuzz , something weird happened, as it seems that the whole file litebird_sim/hwp_sys/hwp_sys.py was changed. Was there a CR/LF conversion that happened underway?

nraffuzz commented 1 year ago

Hi @ziotom78, I saw that, when comparing the old with the new version, two empty lines where missing so that in the end everything was shifted. Probably due to that, everything was counted as something new. However, what it has actually changed is the substitution of sim.parameter_file with sim.parameters, from line 85 to 99 in hwp_sys.py, in order to correctly read the dictionary.

ziotom78 commented 1 year ago

Sorry for the delay in answering you. I found the culprit: you saved the file using DOS line endings instead of Unix (the ones used in the original file). I converted them back, and now the diff now looks reasonable: https://github.com/litebird/litebird_sim/pull/232/files

May you please have a check that the diff is what you expect?

nraffuzz commented 1 year ago

Yes, I confirm that the difference is what I expected, thanks for finding the issue!

ziotom78 commented 9 months ago

Hi, it seems that something weird has happened again… Several files have had their attributes changed: for instance, bin/git/init-hooks has its permission flags changed from 100755 to 100644 (and several others). WIth these changes, the PR reports that ~120 files are modified by this commit. Could you please have a check?

nraffuzz commented 9 months ago

Hi, I didn't specifically get what went wrong, and of course we didn't want to change permission in those files. How can we solve this?

ziotom78 commented 9 months ago

I was able to fix permissions, and now the PR lists just 20 files that have been changed. Does this look reasonable?

nraffuzz commented 9 months ago

@paganol @ziotom78, We (@sgiardie and I) have integrated our respective branches related to hwp systematics into the present branch (PR #267). This module specifically focuses on optical systematics. The following additions and updates have been made:

We believe that this branch is ready for merging into the master. What do you think?

ziotom78 commented 9 months ago

Hi @nraffuzz and @sgiardie , is this PR ready to be merged? I can update the CHANGELOG and do it, just please tell me if there are showstoppers.

nraffuzz commented 9 months ago

No showstoppers from my side, I believe this PR can be merged! Thanks :)

sgiardie commented 9 months ago

Yes, I think it can be merged now!

sgiardie commented 9 months ago

(sorry, I just added a tiny correction to the notebook, now should be perfect!)