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

Add new NIRCam anomalies #1136

Merged
merged 2 commits into from
Dec 21, 2022

Conversation

bhilbert4
Copy link
Collaborator

Resolves #1126

Adds a few new NIRCam anomalies to the list. Note: new columns will need to be added to the anomaly database.

@bhilbert4
Copy link
Collaborator Author

Looks like columns need to be added to the database before the updated page can be viewed on the test server.

@bhilbert4
Copy link
Collaborator Author

@mfixstsci this is ready for review. I should probably learn how to add columns to the database table at some point.

@bhilbert4
Copy link
Collaborator Author

Tests are failing because the MAST server is not available. Maybe I'll wait a bit and try again later.

Copy link
Collaborator

@mfixstsci mfixstsci left a comment

Choose a reason for hiding this comment

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

Thanks @bhilbert4 one question, are these values now reflected in the web app and the in the database? Have you tested this yet?

@bhilbert4
Copy link
Collaborator Author

I tried checking on the test server this morning, but it turns out that the page can't be built until the new columns are added to the database. Until that happens, the exposure page crashes. I don't know yet how to add columns to the database. So we should probably add those columns to all of the databases and have a quick look on the dev or test servers before merging this. Also, #1133 adds some anomalies for NIRISS, so maybe it makes sense to add all the columns together. I'm happy to learn how to add columns to the database.

@mfixstsci
Copy link
Collaborator

@bhilbert4 I just completely ignored this process for NIRISS (better or worse) and then just added the columns to production post merge 😭 Looks like there are some conflicts now if you could resolve them. Let me know what you'd prefer to do. I can update test if you'd like before we merge it.

@bhilbert4
Copy link
Collaborator Author

@mfixstsci I knew that conflict would pop up depending on whether the niriss or nircam PR was merged first. Anyway, it's fixed.

@bhilbert4
Copy link
Collaborator Author

I'm happy to look at it on test if you'd like. I'm pretty confident everything will be ok though (he said, just before the holiday break starts)

@mfixstsci
Copy link
Collaborator

@bhilbert4 thanks for addressing that, I will take a look when the tests pass and just add the columns by hand. I've already done it once today, so why not twice?

@bhilbert4
Copy link
Collaborator Author

Practice makes perfect, right? 😄

Copy link
Collaborator

@mfixstsci mfixstsci left a comment

Choose a reason for hiding this comment

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

LGTM

@mfixstsci mfixstsci merged commit b6d3bc7 into spacetelescope:develop Dec 21, 2022
@bhilbert4 bhilbert4 deleted the add-new-nircam-anomalies branch December 29, 2023 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new anomaly types for NIRCam
2 participants