Skip to content

ML mapmaker I/O#1234

Open
chervias wants to merge 129 commits intomasterfrom
more_ml_mapmaker
Open

ML mapmaker I/O#1234
chervias wants to merge 129 commits intomasterfrom
more_ml_mapmaker

Conversation

@chervias
Copy link
Copy Markdown
Member

This implements the preprocessing module for loading obs into the ml mapmaker, which requires to run preprocessing previously. The idea is to run with LAT data and leave the absolute minimum required preprocessing in this code, and we will remove that eventually

skhrg and others added 30 commits May 5, 2025 17:07
Comment thread sotodlib/mapmaking/utils.py Outdated
Comment thread sotodlib/mapmaking/utils.py
Comment thread sotodlib/mapmaking/utils.py Outdated
passes.append(entry)
return passes

def distribute_tods_ra(obsinfo, nsplit, site="so", weather="typical"):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this work if site="so_lat" instead?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is a perfect candidate for a unit test.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we run it with so_lat in the ML mapmaker. This is a general function so it doesn't matter if the default is so. The difference is tiny.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we supposed to support ACT data as well with SO tools?

Comment thread sotodlib/mapmaking/utils.py Outdated
Comment thread sotodlib/mapmaking/utils.py
Comment thread sotodlib/mapmaking/utils.py
Comment thread sotodlib/mapmaking/utils.py Outdated
Comment thread sotodlib/mapmaking/utils.py Outdated
Comment on lines +40 to +41
parser.add_argument( "--sun-mask", type=str, default="/global/cfs/cdirs/sobs/users/sigurdkn/masks/sidelobe/sun.fits", help="Location of Sun sidelobe mask")
parser.add_argument( "--moon-mask", type=str, default="/global/cfs/cdirs/sobs/users/sigurdkn/masks/sidelobe/moon.fits", help="Location of Moon sidelobe mask")
Copy link
Copy Markdown
Member

@iparask iparask Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I run this anywhere apart from NERSC it will fail. If these inputs are required, remove the defaults and make them require=True

Copy link
Copy Markdown
Member

@skhrg skhrg Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some niceness to defaults that work easily...

I would advocate for the following:

  • Change sidelobes.get_cuts to take the path to a directory where it searches for files with the names {obj_name}.fits for everything in object_list
  • Make object_list a cli argument that defaults to [sun, moon] and pass it into sidelobes.get_cuts
  • Take in the directory to look for files in as a CLI argument but dont require it or set a default
  • If the directory is not passed do something sane to set the default in a what that isn't computer dependent. I would either:
    • Have it assume some relative path like masks/sidelobe (lame and probably not a useful default in most cases)
    • Have it grab the path to the metadata directory (by looking at where context says the obsdb is) and assuming a path relative to that (ie: {metadata_dir}/masks/sidelobe) and we put the current nominal masks there.

That way we end up with a system that doesn't rely on hardcoded paths but is easy to override as needed.

Probably too involved for this PR as it should be merged soon since its been hanging around for a while, but just throwing that out there as an option for future changes.

For now I think its fine that is will crash outside of NERSC, we can just add a warning to the help message that the defaults are for NERSC or something.

Comment thread sotodlib/site_pipeline/make_ml_map.py Outdated
Comment thread sotodlib/site_pipeline/make_ml_map.py

def find_usable_detectors(obs, maxcut=0.1, glitch_flags: str = "flags.glitch_flags"):
ncut = rangemat_sum(obs[glitch_flags])
def find_usable_detectors(obs, maxcut=0.1, glitch_flags: str = "flags.glitch_flags", to_null : str = "flags.expected_flags"):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test?

return sub_ids

def filter_subids(subids, wafers=None, bands=None):
def filter_subids(subids, wafers=None, bands=None, ots=None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this tested?

sub_ids = expand_ids(sub_ids, context=context)
return sub_ids

def get_obsinfo_subids(subids, context):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test?

@chervias
Copy link
Copy Markdown
Member Author

@mhasself there are still a couple of pending issues and unit tests, but you can start to have a look. Thanks!

@chervias chervias marked this pull request as ready for review April 16, 2026 20:52
@chervias chervias requested a review from mhasself April 16, 2026 20:52
chervias and others added 5 commits April 21, 2026 21:30
* Better control over map vs. tod units

* Fix implementation of Jon's noise model. The step where previous eigenvectors were projected out from the covmat used the wrong formula. Also simplified the internal normalization, though this doesn't change anything. Added NmatDebug too. Sotodlib and sogma still don't match when both use Jon's noise model. Currently sotodlib looks quite a bit better
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants