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 demod mapmaker updates #1044

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Conversation

earosenberg
Copy link
Contributor

@earosenberg earosenberg commented Nov 27, 2024

Updates to SAT mapmaking scripts including:

  • Add site_pipeline script to make atomic maps db Write to atomic maps db from the mapmaker
  • Refactor make_atomic_filterbin_map.py to avoiding running get_obs twice
  • Make it possible to provide a wcs_kernel string (e.g. "car-5arcmin") rather than using the --area argument to provide a map filename

mhasself and others added 4 commits November 21, 2024 11:21
obsfiledb time on large file-systems is non-negligible; this cuts time
in half (10 -> 5s on NERSC test case).
This is activated through SOTODLIB_TOD_CACHE envvar.
@earosenberg
Copy link
Contributor Author

@chervias
If we always make per-observation maps, should we use the obsid in the atomic map names instead of period start? This is sometimes confusing as the two often differ slightly.
cf these lines

@chervias
Copy link
Contributor

The actual important time is the ctime on the obsdb, which is when the obs actually started. In the atomic database we have both the obs_id and the path to the maps, so we can match obs and maps with the atomic database, right? I would say we can leave as it is now.

@chervias
Copy link
Contributor

I merged from the branch in #1037 to load obs faster. Please look at that PR for instructions, since an env variable has to be set up to use

@chervias
Copy link
Contributor

The PR is ready for review. Instead of using a separate script for appending to the atomic map DB, now everything is done in the same mapmaker as the maps are being written. Otherwise we have completed the other issues that is PR was meant to solve

@chervias chervias requested review from mhasself and JBorrow December 19, 2024 14:55
@chervias
Copy link
Contributor

@mhasself could you please have a look? @JBorrow could you please have a look at the sqlalchemy bits? Thanks

@chervias chervias marked this pull request as ready for review December 19, 2024 14:57
Copy link
Member

@JBorrow JBorrow 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 comments, happy to discuss further

Comment on lines +544 to +549
obs_id: Mapped[str] = mapped_column(primary_key=True)
telescope: Mapped[str] = mapped_column(primary_key=True)
freq_channel: Mapped[str] = mapped_column(primary_key=True)
wafer: Mapped[str] = mapped_column(primary_key=True)
ctime: Mapped[int] = mapped_column(primary_key=True)
split_label: Mapped[str] = mapped_column(primary_key=True)
Copy link
Member

Choose a reason for hiding this comment

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

Is your aim here to have these be a composite primary key in the database?

Copy link
Member

Choose a reason for hiding this comment

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

Also why use ctime instead of a datetime object?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, these are the things that combined define a unique atomic map. I thought a primary key was something that uniquely defines a row. Therefore, I made all these primary keys. ctime is just our tradition, we define observations by their ctime.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's correct. You can certainly do this. One other option is to have a unique ID as the primary key, but if you are certain that there will never be rows in the database with a duplicate value for these combined items then it is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. If I try to add another row to the database with the same set of these primary keys, it will error, right? That should never happen because these same keys are the ones used to check for existing maps at the beginning of the mapmaker. That way you don't re-map and existing map.

Comment on lines 402 to 407
info = mapmaking.AtomicInfo(
obslist[0][0],
obs_infos[obslist[0][3]].telescope,
band,
detset,
int(t))
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend using kwargs here instead of positional

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate a bit? I do not understand what you mean. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I would have written:

info = mapmaking.AtomicInfo(
    obs_id=obslist[0][0],
    telescope=obs_infos[obslist[0][3]].telescope,
    ...
)

This allows you to change the order of parameters, etc, later without impacting this code in potentially invisible ways.

Comment on lines 408 to 414
info.split_label = split_label
info.split_detail = ''
info.prefix_path = str(prefix + '_%s' % split_label)
info.elevation = obs_infos[obslist[0][3]].el_center
info.azimuth =obs_infos[obslist[0][3]].az_center
info.pwv = float(pwv_atomic)
info_list.append(info)
Copy link
Member

Choose a reason for hiding this comment

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

Why not just do this in the initializer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I could do that in the init, but I reserved that for the primary keys that are mandatory, and then keep adding the optional things.

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.

4 participants