Draft: Add support to HWP in totalconvolve #555
Conversation
mreineck
changed the title
|
To be merged after #552 |
src/toast/ops/totalconvolve.py
Outdated
| @@ -277,6 +277,13 @@ def _exec(self, data, detectors=None, **kwargs): | |||
| beam = self.get_beam(beam_file, lmax, mmax, det, verbose) | |||
|
|
|||
| theta, phi, psi, psi_pol = self.get_pointing(data, det, verbose) | |||
| np.savez( | |||
|
please make sure that you fetched the latest changes in #556 .. |
|
Hi folks, I am about to merge a PR (#560) that has some tedious whitespace changes (running isort on the source tree). It looks like this branch had the |
|
Hi again, I think I see why the unit tests "passed". If ducc0 is not available, these tests are skipped. Our upstream docker containers do not have this package. I'll go fix that and then we can re-test this branch. |
|
The totalconvolver tests passed on my local machine, with ducc0 installed, and they they took too long to have been skipped. So I"m still somewhat optimistic. Concerning rebasing: unfortunately all my other projects don't use this approach, so I'm really inexperienced in that respect. I'll try to get up to speed with this. In any case, I think I may have understood now how to implement fully general HWP convolution support. With a bit of luck I'll have an implementation for that soon, which might make the current totalconvolver patch obsolete. |
|
Just a note that I have added About merging vs rebasing, the nice features of rebasing are that it is easy to move around the history and revert commits without crossing merge commits (where you have to decide which branch to take). However, technically only branches on a fork (i.e. private branches) should be rebased since it invalidates everyone's checked out copy. Since no one else has a checkout of your branches on your fork, it is no problem. For developers with push access to this repo, we have typically been lazy (myself included!) and have used branches in this main repo as "private" where we rebase at will and assume no one else has checked them out. It saves some overhead instead of synchronizing a fork with the upstream repo, but technically this is a bad practice. For branches with only one person working on it there is no problem, but if multiple people have made commits to a branch then rebasing becomes dangerous and should not be done without closely coordinating with everyone working on that branch (since they have to delete and re-checkout their local copy). Since you have already done a merge of the upstream branch into your development branch, then you should not try to rebase it. Just continue to merge upstream toast3 into your branch. Mixing the two techniques will cause you many headaches, perhaps even forcing you to copy the changed files out of the way and make a new branch. Some larger projects (e.g. astropy) are picky about only accepting PRs that have been rebased against the current upstream commit. We don't currently have enough developers for such things to matter too much, but at some point should be more strict about our rebasing of public branches in the main toast repo. |
* Introduce a small helper class for tracking observing sessions. * Disable totalconvolve unit tests, to be fixed in PR #555
This is an attempt to port the TEB convolution changes from conviqt to totalconvolve.
Surprisingly, the tests appear to work, but please have a very careful look before merging this, since in many places I don't really know what I am doing ...
I'm fine with merging the tests for
totalconvolveinto a single file at some point; for the moment I just wanted to make sure not to lose the old tests and also have a file that can be easily compared to the updated conviqt tests.Please let me know if there are still missing/broken things!