Conversation
|
Looks like some other code needs to be updated to support the new FocalPlane class that replaced the old OrderedDict. Some places expect to be able to iterate over |
…acing asm.dets with asm.fplane.quats
|
This pull request is now backwards compatible with the current version of sotodlib. Let's get it merged so I can get my new sotodlib branch merged! It's needed for my demod_ml_mapmaker for example. |
…aking one with just the boresight
mhasself
left a comment
mhasself
left a comment
There was a problem hiding this comment.
Thanks for this, and for the iterations (offline) on the docs. Couple minor things inline... the only larger concern is whether focalplane.quats should be an array, or ... quats.
| [array([[ 0.25715457, -0.92720643, 0.9999161 , 0.01295328]]), | ||
| array([[ 0.24261138, -0.92726871, 0.86536489, 0.5011423 ]]), | ||
| array([[ 0.22806774, -0.92722945, 0.50890634, 0.86082189]])] |
There was a problem hiding this comment.
The first sentence, below this, would appear now to be false.
There was a problem hiding this comment.
Fixing. We should check whether its sin(gamma) or sin(2*gamma) at some point too. I'm leaving in the sin(gamma) for now, since I don't know that it's wrong.
| # Get vector of quaternion pointings for detector 0 | ||
| q_total = csl.Q * spt3g.core.quat(*fp.quats[0]) | ||
| # Decompose q_total into lon, lat, roll angles | ||
| ra, dec, gamma = so3g.proj.quat.decompose_lonlat(q_total) |
There was a problem hiding this comment.
So why is focal_plane.quats an array instead of a G3VectorQuat? The quat math is really handy, and it will usually be more work for a user to dig up the quat / G3VectorQuat class for use as a cast, than it would be if someone just wanted to turn the quats into an array.
In toast/quaternion_array, numpy arrays are used to hold quaternions but in the opposite convention (real component last), so using a special quat object is also a way to be explicit about the convention.
There was a problem hiding this comment.
I thought about the same thing when preparing the pull request. I think the reason was that the C++ code needs a numpy array. Ideally G3VectorQuat would have the same memory layout as the corresponding numpy array of coefficients, so conversion would be free, but as it is, one has to do a conversion any time one wants to use any pointing operation. I wanted the internal representation to have no impedance mismatch with the low-level pointing code, and that's why I ended up with the numpy array.
That said, this is a small array. The conversion cost will be completely negligible unless we have very few samples. Let's just switch to it being a G3VectorQuat and eating the conversion cost.
python/proj/coords.py
Outdated
| # response too. Writing it like this handles both cases, and | ||
| # as a bonus(?) any combination of them | ||
| # FIXME: old sotodlib compat - remove later | ||
| xi, eta, gamma, T, P, Q, U, hwp, dets = cls._xieta_compat(xi,eta,gamma,T,P,Q,U,hwp) |
There was a problem hiding this comment.
Does not quite work ... the 4th argument changes from "gamma=0" to "T=1". So old syntax that does not pass gamma will have gamma=1. Fix is to have default T=None in signature.
This might not actually affect any code that's out there...
There was a problem hiding this comment.
I fixed it in a more direct way by manipulating *args and **kwargs directly, basically just removing the first element of args.
python/proj/coords.py
Outdated
| """Slice the FocalPlane with slice sel, resulting in a new | ||
| FocalPlane with a subset of the detectors. Only 1d slice supported, | ||
| not integers or multidimensional slices. Example: ``focal_plane[10:20]`` | ||
| would make a sub-FocalPlane with detector indices 10,11,...,19. |
There was a problem hiding this comment.
Mention that 1d index array, and boolean mask selection arrays (my favorite) also work fine.
python/proj/coords.py
Outdated
| If fplane is not None, then the result will be | ||
| [n_samp,{lon,lat,cos2psi,sin2psi}]. Otherwise it will | ||
| be [n_det,n_Samp,{lon,lat,cos2psi,sin2psi}] |
There was a problem hiding this comment.
Remove "not".
n_Samp -> n_samp
| t, az, el = get_scan() | ||
| csl = proj.CelestialSightLine.az_el(t, az, el, weather='vacuum', site='so') | ||
| fp = proj.FocalPlane.from_xieta(['a', 'b'], [0., .1*DEG], [0, .1*DEG]) | ||
| fp = proj.FocalPlane.from_xieta([0., .1*DEG], [0, .1*DEG]) |
There was a problem hiding this comment.
Also please fix demos/demo_proj_hp.py ... this does it:
--- a/demos/demo_proj_hp.py
+++ b/demos/demo_proj_hp.py
@@ -191,13 +191,6 @@ def simulate_detectors(ndets, rmax, seed=0):
eta = rr * np.sin(phi)
return xi, eta, gamma
-def make_fp(xi, eta, gamma):
- fp = so3g.proj.quat.rotation_xieta(xi, eta, gamma)
- so3g_fp = so3g.proj.FocalPlane()
- for i, q in enumerate(fp):
- so3g_fp[f'a{i}'] = q
- return so3g_fp
-
def extract_coord(sight, fp, isignal=0, groupsize=100):
coord_out = []
for ii in range(0, len(fp), groupsize):
@@ -207,15 +200,14 @@ def extract_coord(sight, fp, isignal=0, groupsize=100):
return coord_out
def make_signal(ndets, rmax, sight, isignal, seed=0):
- xi, eta, gamma = simulate_detectors(ndets, rmax, seed)
- fp = so3g.proj.quat.rotation_xieta(xi, eta, gamma)
+ fp = so3g.proj.FocalPlane.from_xieta(*simulate_detectors(ndets, rmax, seed))
signal = extract_coord(sight, fp, isignal)
return signal
def make_tod(time, az, el, ndets, rmax, isignal, seed=0):
sight = so3g.proj.CelestialSightLine.naive_az_el(time, az, el)
signal = make_signal(ndets, rmax, sight, isignal, seed)
- so3g_fp = make_fp(*simulate_detectors(ndets, rmax, seed))
+ so3g_fp = so3g.proj.FocalPlane.from_xieta(*simulate_detectors(ndets, rmax, seed))
asm = so3g.proj.Assembly.attach(sight, so3g_fp)
return signal, asm
mhasself
left a comment
mhasself
left a comment
There was a problem hiding this comment.
Last couple of minor things... but approving.
Squash this on merge!!
python/proj/coords.py
Outdated
| quats: G3VectorQuat representing the pointing quaternions for | ||
| each detector. Can be turned into a numpy array of coefficients | ||
| with np.array(). Or call .coeffs() to get them directly. | ||
| resps: Array of float64 with shape [ndet,2] representing the |
python/proj/coords.py
Outdated
| """This class collects the focal plane positions and orientations for | ||
| multiple named detectors. The classmethods can be used to | ||
| construct the object from some common input formats. | ||
| Members: |
There was a problem hiding this comment.
I'm not sure if sphinx likes "members" -- can you change to "Attributes:"?
| def from_xieta(cls, *args, **kwargs): | ||
| """ | ||
| def from_xieta(cls, xi, eta, gamma=0, T=1, P=1, Q=1, U=0, hwp=False): | ||
| Construct a FocalPlane from focal plane coordinates (xi,eta). |
There was a problem hiding this comment.
Mention "for backwards compatibility, the signature from_xieta(names, xi, eta, gamma=0) is also supported; but this will be removed in the future."
mhasself
left a comment
mhasself
left a comment
There was a problem hiding this comment.
Hooray! So stoked!
Squash on merge!
3 participants
Edit: No longer breaks backwards compatibility - see jan 10 comment.
The pointing matrix in so3g has had a hardcoded polarization efficiency of 100%. This pull request updates it to support arbitrary T and P responses per detector. This was needed to be able to represent demodulated timestreams as virtual detectors, which will be a separate pull request for sotodlib later. The virtual detector approach to demodulation mapmaking has the advantage that none of the rest of the code needs to know about the demodulation, only the demodulation function itself.
This breaks backwards compatibility in the interface of the FocalPlane object. It used to be an OrderedDict of
[name, q]where q is an entry in aG3VectorQuat, but it is now a class with the members.quats, which is an [ndet,4] array of quaternion coefficients, and.resps, which is an[ndet,2]array of T and P responses. This means that FocalPlane no longer supports named detectors. This simplifiesAssembly, which now just contains a FocalPlane instance.This incompatibility will require corresponding changes in sotodlib. Do we have to go through a slow dance of add compatibility layer in sotodlib, wait for a few weeks, update so3g, then remove compatibility layer in sotodlib, again? I hope not. What do you think?