Fix reindexing of fixed tones#1625
Conversation
|
@tpsatt Thanks for working this out. Since unit tests on this kind of thing are pretty tricky, can you mention in the PR comments here what obs_id(s) you used for testing? |
|
@mhasself Good point. For testing, I was using |
ykyohei
left a comment
ykyohei
left a comment
There was a problem hiding this comment.
This looks reasonable to me, and I tested to run update_det_cal site-pipeline with this branch and confirmed that there is no problem, just in case.
This reindexing was used for update_det_cal with hwpss_subtraction for SATs. update_det_cal should not have had issues because it analyzes oper books that is saved per wafer.
| @@ -877,7 +877,7 @@ def reindex_axis(self, axis, indexes, in_place=True): | |||
| shape.append(s) | |||
|
|
|||
| new_v = np.empty(shape, dtype=v.dtype) | |||
There was a problem hiding this comment.
Can we use zeros, or zeros() - 1, instead of empty here?
The idea that "empty" will always produce garbagey results that are easy to distinguish from "good numbers" is not accurate. Sometimes "empty" will give you 0, or -1, or other normal numbers.
So I'd rather we have the predictability of a simple result here, such as zero or -1 for integers, than to rely on whatever garbage is given by empty.
There was a problem hiding this comment.
Good point. I've just pushed a change which sets floats to NaN and ints to -1. I think this is minimally invasive since many bad float values in "real" data (e.g. R_ns) are already NaNs, and bad ints (e.g. bias lines) are -1s. This also preserves strings being ''.
There was a problem hiding this comment.
Thanks. Given that new_v is now simply replaced in the int and float specializations, you can skip the .empty creation here and instead do a .zeros in an "else" clause for the type checks.
I really don't like leaving .empty arrays lying around! Non-determinism.
There was a problem hiding this comment.
Sometimes we have strings in the data, so I like that empty becomes an empty string. When I tried to put a case in for this, it broke (I think because of how numpy encodes strings, I'm not an expert here)
There was a problem hiding this comment.
.zeros has always worked on strings for me ... also returns an array of ''s ... What's the dtype that fails?
There was a problem hiding this comment.
I see your point now. I've updated this and committed. Thanks!
|
@ykyohei should probably review again since I've tweaked the reindexing function |
|
The unit test failures here are due to an interplay between the latest toast wheels and the so3g wheels. I'm debugging in a separate PR. |
|
I tested update_det_cal again and I didn't find problem. |
4 participants
Loading an observation with fixed tones (
special_channels) and reindexing was broken because multiple UFMs (stream_ids) could share the same band/channel numbers. This fixes that, and fixes a bug where NaNs were not propagated in the reindexing.