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

Update readnoise monitor to use django database models #1482

Merged
merged 11 commits into from
Feb 15, 2024

Conversation

york-stsci
Copy link
Collaborator

This PR is a test of updating one of the monitors (readnoise) to use the django database model, both for insertion and retrieval. It also adds general documentation describing the process.

@pep8speaks
Copy link

pep8speaks commented Feb 14, 2024

Hello @york-stsci, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2024-02-14 21:36:58 UTC

Copy link
Collaborator

@bhilbert4 bhilbert4 left a comment

Choose a reason for hiding this comment

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

This looks ok to me, as someone who is not a Django model expert. And I'm assuming there's no issue with defining the models for all of the monitors in this PR, even though the monitors themselves are not yet switching over to use them.

I do think the Django environment variable setting and call to setup() probably need to be in an if clause so that they're not run on RTD or GA.

@york-stsci
Copy link
Collaborator Author

The models were actually all defined in a previous (and merged) PR. This PR touches all of those files because, when I found out that I needed to change the (incorrect) data types that django had assigned to array fields, I just did it for all the monitors rather than stop with readnoise.

Copy link
Collaborator

@BradleySappington BradleySappington left a comment

Choose a reason for hiding this comment

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

This looks good to me with only a mispelling in the doc.
Aside from readnoise.py, I did not dive deep into the monitor_models as they will be changing.

Especially excellent work with the documentation update, thanks for taking the time to do that!

docs/source/database.rst Outdated Show resolved Hide resolved
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.

Hey @york-stsci just a few questions but this is huge improvement, thank you for submitting this!

@mfixstsci mfixstsci merged commit f3a19b4 into spacetelescope:develop Feb 15, 2024
6 checks passed
@york-stsci york-stsci deleted the update_readnoise branch February 16, 2024 17:17
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.

5 participants