Adding unfied planet mapmaker using for MF-SATs - #1706
Conversation
| """ | ||
| # This is based on TOD fits summarized here: https://docs.google.com/presentation/d/1DPBeMX9ZPHwQyi9-s5orfRZIYcRLXaejFWOtlsflgUU/edit?usp=sharing | ||
| # amplitude below is in arcmin, phase in radian | ||
| amp_satp1 = {'f090': {'ws0': 0.3945, |
There was a problem hiding this comment.
these sorts of hard-codings I would move to a version-controlled configuration file.
mhasself
left a comment
There was a problem hiding this comment.
Thanks for all this work! As requested, here is some general guidance (but not an exhaustive review yet):
- Please include an example config file as part of documentation.
- Format code and docs to less than 120 columns.
- There are also some "whitespace" violations -- might be worth just running a source formatter on the new files you're contributing (but don't mass-reformat the existing files like planets.py!)
- Consider making the deflection/wobble code a little more general, and moving into coords module somewhere.
- You have functions for getting and archiving and processing PWV. Are those needed, if instead we can provide PWV through obsdb? I think it's good to minimize the number of different ways we pull data from APEX / Toco radiometer.
- Use "single" instead of "each" for functions that process one of something (e.g.
get_each_instrument_P). "Each" implies that it is looping, internally, over a bunch of things. - Remove default paths from planet_mapmaker.py funcs
Much of the new code in planets.py is basically dealing with the fact that SlowSource is not really good enough, at this point. It would be nice to take your functions calc_planet_* and instead create a replacement for SlowSource, e.g. AzElSource, that provides an az, el interpolator for any kind of source with sufficient precision for beams work. (Basically I want to get rid of SlowSource in favor of something that works better, without a huge performance penalty for simple cases.)
| tags = obs['tags'] | ||
| subs = 'ws' | ||
| if configs['query'].get('all_wafers', False): | ||
| obs_wafers = [f'ws{i}' for i in range(7)] |
There was a problem hiding this comment.
Perhaps use obs['wafer_slots_list'] for this.
| # add function for overwrite | ||
| for wafer in obs_wafers: | ||
| if configs['overwrite']: | ||
| irunlist = {'obs_id':obs_id, 'wafer_info': {'wafer_slot': wafer, 'wafer.bandpass': band}} |
There was a problem hiding this comment.
Construct this data one time (instead of duplicating 3 times).
| @@ -0,0 +1,138 @@ | |||
| # To make atmic planet map in detector-centered coordinate system | |||
There was a problem hiding this comment.
Use a full submodule docstring """ ... """ here, and explain in more detail what the script does. It's also a good place to put in an example of a configuration file. The docstring here is easy to have rendered in readthedocs, so it can be the "one place" this script is explained.
| 'det_weights': det_weights, | ||
| } | ||
|
|
||
| def xieta0(q): |
There was a problem hiding this comment.
Ah, xieta decomposition won't work (as you say in docstring), but I think this would?:
phi, theta, gamma = so3g.proj.quat.decompose_iso(q)
return so3g.proj.rotation_iso(phi, theta, 0.)
One more thing: utility functions (such as the ones you've added in |
Adding general functions in coords.planets.py and functions related to planet mapmaking in mapmaking.planet_mapmaker.py.
Now is a very very draft, but I confirmed that a planet map was made. Will arrange these more appropriately.