ML Mapmaker fixes #1404
ML Mapmaker fixes #1404
Conversation
sotodlib/mapmaking/utils.py
Outdated
| def find_usable_detectors(obs, maxcut=0.1, glitch_flags: str = "flags.glitch_flags", to_null : str = "flags.expected_flags"): | ||
| flag = obs[glitch_flags] | ||
| if to_null != "": | ||
| flag *= ~obs[to_null] |
There was a problem hiding this comment.
Won't this fail if the obs doesn't have flags.expected_flags? We either need to artificially add those empty flags before in the main script or leave the default of to_null to be an empty string
| parser.add_argument("-T", "--tiled" , type=int, default=1, help="0: untiled maps. Nonzero: tiled maps") | ||
| parser.add_argument( "--srcsamp", type=str, default=None, help="path to mask file where True regions indicate where bright object mitigation should be applied. Mask is in equatorial coordinates. Not tiled, so should be low-res to not waste memory.") | ||
| parser.add_argument( "--unit", type=str, default="uK", help="Unit of the maps") | ||
| parser.add_argument( "--maxcut", type=float, default=.1, help="Maximum fraction of cut samples in a detector.") |
There was a problem hiding this comment.
I think I would like the default to be 0.3
| from sotodlib.core import metadata | ||
| from pixell import enmap, utils, fft, bunch, wcsutils, mpi, bench | ||
| import yaml | ||
| from mpi4py import MPI |
There was a problem hiding this comment.
why is this needed? mpi is loaded through pixell, which is also mpi4py
…fault to planets module for recenter if possible
… and take in usable detector thresh as a cli arg
…es requested by carlos
|
Okay I think other than the quaternion change this should be good ot merge now. @mhasself any shot you could take a look at that? Separately I would like extend the center-at stuff to add any other useful recenterings we can think of (a few have been brought up with the moon centered maps that I don't think we can do currently) but that can be a separate PR. |
| parser.add_argument("prefix", nargs="?") | ||
| parser.add_argument( "--comps", type=str, default="TQU",help="List of components to solve for. T, QU or TQU, but only TQU is consistent with the actual data") | ||
| parser.add_argument("-W", "--wafers", type=str, default=None, help="Detector wafer subsets to map with. ,-sep") | ||
| parser.add_argument("-O", "--ots", type=str, default=None, help="Opics tubes to map with. ,-sep") |
| ra0, dec0 = planet.pos(tod.timestamps.mean()) | ||
| except: | ||
| obj = getattr(ephem, name)() | ||
| djd = ctime/86400 + 40587.0 + 2400000.5 - 2415020 |
There was a problem hiding this comment.
What are all these numbers? It would be better if you include them in variables or constants that we can import
| if np.any(nkept_all == 0): | ||
| group = comm.Get_group() | ||
| new_group = group.Incl(np.where(nkept_all > 0)[0]) | ||
| new_comm = comm.Create(new_group) | ||
| if nkept == 0: | ||
| L.info("No tods assigned to this process. Pruning") | ||
| sys.exit(0) | ||
| comm = new_comm |
There was a problem hiding this comment.
Can you move this to a function? It changes the mapmaker's comm, which is not a mapmaking specific operation. I think it should go to utils in case some else wants to use it.
|
Okay pending weird coordinate stuff this should be good to go. |
sotodlib/mapmaking/utils.py
Outdated
| if mapmaker is not None: | ||
| for signal in mapmaker.signals: | ||
| if hasattr(signal, "comm"): | ||
| signal.comm = comm |
There was a problem hiding this comment.
This looks much better. I do not think the mapmaker should be in here. This function returns the comm and it should remain as such. So just pull the mapmaker outside and it would be great!
So it will look like:
pruned_comm = mapmaking.prune_mpi(comm, np.where(nkept_all > 0)[0])
for signal in mapmaker.signals:
if hasattr(signal, "comm"):
signal.comm = pruned_comm
| return lonlat | ||
| elif sys == "hor": | ||
| return so3g.proj.CelestialSightLine.az_el(ctime, lonlat[0], lonlat[1], site=site, weather=weather).coords()[0,:2] | ||
| return so3g.proj.CelestialSightLine.az_el(ctime, -1*lonlat[0], np.pi/2 - lonlat[1], site=site, weather=weather).coords()[0,:2] |
There was a problem hiding this comment.
The pi/2 - el still looks wrong, so run this by @amaurea or otherwise figure out what's up.
Some minor fixes and QOL updates to the ML mapmaker.
These are mostly non-invasive but the quaternion change in the recentering code needs verification (will actually produce maps now but that could be true even if I messed it up).