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

Sat mapmaking refactor #973

Merged
merged 50 commits into from
Nov 8, 2024
Merged

Sat mapmaking refactor #973

merged 50 commits into from
Nov 8, 2024

Conversation

chervias
Copy link
Contributor

First step of the refactoring of the SAT mapmaking code, which is the general function to make a demodulated map (either atomic or depth-1). This will live in mapmaking.demod_mapmaker.

@chervias
Copy link
Contributor Author

chervias commented Sep 28, 2024

Also, MPI is being removed from the functions in demod_mapmaker, because that is only useful for making depth-1 maps which we are not doing now, so for the time being its just annoying. Depth-1 mapmaking is still supported, just running in a single job. (EDIT: this is not true anymore, we will keep the mpi support but use a fake comm)

@chervias chervias marked this pull request as ready for review October 11, 2024 11:50
@chervias chervias mentioned this pull request Oct 25, 2024
@chervias chervias requested a review from mhasself October 29, 2024 18:44
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.

Minor comments -- thanks.

@@ -16,7 +16,7 @@
"""

__all__ = ['build_obslists']
import numpy as np
import numpy as np, sys
Copy link
Member

Choose a reason for hiding this comment

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

For imports, each module is a separate line.

Looking further below, why is sys.exit used in a support function? Raising an exception would be more Pythonic.

Comment on lines 386 to 389
"""
Setup the classes for demod mapmaking and return
a DemodMapmmaker object
"""
Copy link
Member

Choose a reason for hiding this comment

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

Please format your docstrings a bit more consistently with the rest of sotodlib. The extra indentation is not our practice, should be:

    """
    Setup the ...

Same for make_demod_map, below.

L : logger, optional
Logger for printing on the screen.
site : str, optional
Plataform name for the pointing matrix.
Copy link
Member

Choose a reason for hiding this comment

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

Platform

from .. import coords
from .. import site_pipeline
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like that library code imports things from site_pipeline. That module is sort of special. For now can you import that submodule from inside your driver function?

The load_or_preproc function, or whatever it's called, should probably move to library level... that's beyond the scope of this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I agree, probably this function should be in core next to get_obs. @msilvafe I will create an issue to remind us

@chervias chervias requested a review from mhasself November 8, 2024 09:23
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.

Thanks for the fixes!

@mhasself mhasself merged commit a03ad3b into master Nov 8, 2024
4 checks passed
@mhasself mhasself deleted the sat-mapmaking-refactor branch November 8, 2024 14:37
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.

3 participants