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

Migrations for recent sdm_schemas: psf, pixelscale, mountjitter, magnitude limits #52

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

Vebop
Copy link
Collaborator

@Vebop Vebop commented Nov 12, 2024

Added migrations for minor changes in latiss, lsstcomcam,sim and added startracker tables:
add exposure quicklook to latiss and updates including pixelscale psf mount jitter and magnitude limits

@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review November 13, 2024 18:14
schema="cdb_latiss",
)
op.add_column(
"exposure",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know that these are being added to the Header Service? Where's the ticket for adding these to hinfo? If the source is not Header Service/hinfo, these should not be in the exposure table. (And we should have caught this earlier in the review of the sdm_schemas PR.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We talked and agreed there was no plan for these; expect them removed in the next push.
Should we also remove from sdm_schemas so they do not get continually readded?

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked and agreed there was no plan for these; expect them removed in the next push.

Should we also remove from sdm_schemas so they do not get continually readded?

Removal of columns should be managed through sdm_schemas PRs, and then these changes will be used to generate the migrations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We didn't update this comment thoroughly, we did adjust through sdm_schemas PR here: lsst/sdm_schemas#284

),
schema="cdb_latiss",
)
op.drop_column("visit1_quicklook", "postisr_pixel_median", schema="cdb_latiss")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we decided that this column hadn't been used so this isn't a backwards-compatibility issue, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there data in this table yet? It doesn't seem so from my cursory queries; so there would be no incompatibility then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And in lsstcomcam, there are only 'postisr_pixel_median_median', and mean, and max.
For latiss, this value is in exposure_quicklook, so I think I agree this is a duplicate of info as well.

@JeremyMcCormick
Copy link
Contributor

JeremyMcCormick commented Nov 16, 2024

@Vebop Does this get the consdb migration scripts completely up to date with the current cdb schemas on sdm_schemas main? Or are there still some updates that are missing?

I just wanted to verify (ticket description seems to indicate that this should cover all outstanding migrations), because we shouldn't turn on the automatic migrations added by DM-47507 until we are completely caught up.

@Vebop Vebop force-pushed the tickets/DM-47443 branch 2 times, most recently from 53a5f74 to e0380e3 Compare November 20, 2024 01:42
@Vebop
Copy link
Collaborator Author

Vebop commented Nov 25, 2024

@JeremyMcCormick Yes I believe this does account for all updates that have been included in sdm_schemas up to this point. I will make sure you know when these have been applied to the summit and USDF so we can set the automatic process to play.

@Vebop Vebop requested a review from ktlim November 25, 2024 23:49
Copy link
Contributor

@ktlim ktlim left a comment

Choose a reason for hiding this comment

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

I didn't formally approve before, but this looks good now (and is being deployed as I write...).

@Vebop
Copy link
Collaborator Author

Vebop commented Nov 27, 2024

Thanks for the approve! Rebased, then will merge after the checks complete.

@Vebop Vebop merged commit 593b437 into main Nov 28, 2024
7 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.

3 participants