Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detector Calibration Refactor #942

Merged
merged 20 commits into from
Sep 23, 2024
Merged

Detector Calibration Refactor #942

merged 20 commits into from
Sep 23, 2024

Conversation

jlashner
Copy link
Contributor

@jlashner jlashner commented Sep 4, 2024

This PR adds the update_det_cal.py site pipeline script. This contains the det-cal update logic that was was previously in update_smurf_caldbs, along with the ability to run the TES parameter corrections documented here.

Due to the time consuming nature of the detcal correction and its fitting, some care had to be taken in the parallelization process. I've found that there are different optimal ways to parallelize this depending on whether you're running at NERSC or on site computing. IO to NERSC sqlite files is slow, so it is beneficial to parallelize the the obs-info loading in addition to the correction computation. The NERSC method chains together two separate multiprocessing pools, one for sqlite access and the other for param correction, and allows you to set the number of processes in each pool individually.

On site computing, sqlite IO is faster, so we don't need to parallelize the sqlite access. Instead we create a pool of however many processes we want, and run through observations serially, using the pool to parallelize the TES param corrections by running each detector correction in a separate process.

The method run is settable via the cfg object / file.

We can still run without the calibration correction by setting cfg.apply_cal_correction = False.

The TES param correction code itself is in sodetlib here.

Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

As requested in conversation:

  • add to docs
  • add to __init__.py
  • deal with the fact that sodetlib is now a site-pipeline dependency
  • consider including the "naive" versions of the fracRn / responsivity in the dataset, as well
  • fix main() and arg parsing to be site_pipeline / prefect compliant.

Thanks!

@jlashner
Copy link
Contributor Author

jlashner commented Sep 9, 2024

@mhasself do you think you can point out the init.py file I need to change? I don't see where in the site pipeline init or the sotodlib init I should be adding something.

@mhasself
Copy link
Member

mhasself commented Sep 9, 2024

@mhasself do you think you can point out the init.py file I need to change? I don't see where in the site pipeline init or the sotodlib init I should be adding something.

Ah, I was misremembering. It's actually in so-workflow (here). So, not your job right now.

For compatibility with Prefect and the so-site-pipeline CLI, see https://sotodlib.readthedocs.io/en/latest/site_pipeline.html#module-sotodlib.site_pipeline.cli. Adding stuff to site_pipeline/cli.py should break the CI, as desired...

@jlashner
Copy link
Contributor Author

jlashner commented Sep 9, 2024

Thanks for the comments @mhasself, I think I went through and got everything. I need to rebuild the database with the naive estimates though.

Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

A few minor complaints -- once addressed feel free to squash + merge.

root_dir: str,
context_path: str,
*,
data_root: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Remove spaces around "=" in default value specs for functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did run this through an auto-formatter (ruff), and it prefers spaces around arguments when type hints are present. Looking at the pep-8 page, it looks like they actually say this is correct and spaces around the equal sign is preferred when using type annotations. Is this ok with you?

Copy link
Member

Choose a reason for hiding this comment

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

So it does! Ok with me.

sotodlib/site_pipeline/update_det_cal.py Show resolved Hide resolved
sotodlib/site_pipeline/update_det_cal.py Outdated Show resolved Hide resolved
sotodlib/site_pipeline/update_det_cal.py Outdated Show resolved Hide resolved
@jlashner jlashner merged commit 0f0c0c0 into master Sep 23, 2024
5 checks passed
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.

2 participants