Repository navigation
Ys/planet db - #38
Ys/planet db#38yoshinori-0778 wants to merge 13 commits into
Conversation
9fd500b to
c6b73f3
Compare
JBorrow
left a comment
JBorrow
left a comment
There was a problem hiding this comment.
I'd like to see more documentation on the fields and a potential consideration of creating more tables that are linked to keep individual row size smaller.
| ctime: AstroPydanticTime | ||
| dtime: datetime |
There was a problem hiding this comment.
We generally like to keep all times internally as astropydantic times.
There was a problem hiding this comment.
Got it, so should I remove dtime?
| obs_id: str = Field(primary_key=True) | ||
| telescope: str = Field(primary_key=True) | ||
| freq_channel: str = Field(primary_key=True) | ||
| wafer: str = Field(primary_key=True) | ||
| ctime: float = Field(nullable=False, primary_key=True) | ||
| source: str = Field(primary_key=True) |
There was a problem hiding this comment.
This is a very large composite primary key. I would have thought that the obs_id would be unique enough, but I guess it may not be?
There was a problem hiding this comment.
yes, obs_id is not unique enough. I realize that ctime is actually not needed to be primary_key. I will remove that. I would like to keep other keys.
| telescope: str = Field(primary_key=True) | ||
| freq_channel: str = Field(primary_key=True) | ||
| wafer: str = Field(primary_key=True) | ||
| ctime: float = Field(nullable=False, primary_key=True) |
There was a problem hiding this comment.
You have redeclared ctime from a time to a float here.
| redchiu: float | None | ||
|
|
||
|
|
||
| class PlanetMapTable(SQLModel, table=True): |
There was a problem hiding this comment.
It would be great to have some documentation on some of these fields.
| # Main beam fit result | ||
| amplitude: float | None | ||
| peak: float | None | ||
| xo: float | None | ||
| yo: float | None | ||
| sigmax: float | None | ||
| sigmay: float | None | ||
| theta: float | None | ||
| redchit: float | None |
There was a problem hiding this comment.
Is it possible that we could have this as a separate link table? That's especially useful if a separate process comes by and does the beam fitting at a later time, or the beam fitting might be re-done several times. See e.g. https://github.com/simonsobs/mapcat/blob/main/mapcat/database/pointing_residual.py
There was a problem hiding this comment.
Sure, I will make a separte link for main beam and leakage beam.
|
@JBorrow I think I can address your comments, so could you review this again? |
| # Main beam fit result | ||
| amplitude: float | None | ||
| peak: float | None | ||
| xo: float | None | ||
| yo: float | None | ||
| sigmax: float | None | ||
| sigmay: float | None | ||
| theta: float | None | ||
| redchit: float | None | ||
| # Leakage beam fit result | ||
| mq: float | None | ||
| d0q: float | None | ||
| d1q: float | None | ||
| sigmaq: float | None | ||
| redchiq: float | None | ||
| mu: float | None | ||
| d0u: float | None | ||
| d1u: float | None |
There was a problem hiding this comment.
You still have the fit result in this model
|
You should also add a test that populates a row with synthetic data and attempts to delete it. |
|
You need to add an alembic migration to add your tables: https://github.com/simonsobs/mapcat/tree/main/mapcat/alembic/versions. AI is very good at doing that. |
This branch has not been deployed
2 participants
This PR adds database for planet maps and todfit. These are used in planet mapmaker in sotodlib (simonsobs/sotodlib#1736) for MF-SATs.