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

NIRCam Claw Monitor #1152

Merged
merged 31 commits into from
Dec 18, 2023
Merged

Conversation

bsunnquist
Copy link
Collaborator

This PR makes the NIRCam Claw Monitor, which creates observation-level stacks of all NIRCam imaging data to detect claws and other scattered light/persistence issues. These stacks can also double as a rough flat field check for those observations with longer exposure times/higher backgrounds.

@bsunnquist bsunnquist self-assigned this Jan 10, 2023
@bsunnquist bsunnquist linked an issue Jan 10, 2023 that may be closed by this pull request
@pep8speaks
Copy link

pep8speaks commented Sep 28, 2023

Hello @bsunnquist, Thank you for updating !

Line 153:121: W504 line break after binary operator
Line 154:107: W504 line break after binary operator
Line 363:17: E722 do not use bare 'except'

Line 994:5: W503 line break before binary operator
Line 995:5: W503 line break before binary operator
Line 996:5: W503 line break before binary operator
Line 997:5: W503 line break before binary operator
Line 998:5: W503 line break before binary operator
Line 999:5: W503 line break before binary operator

Line 161:1: W293 blank line contains whitespace

Comment last updated at 2023-12-18 20:57:37 UTC

@bsunnquist bsunnquist requested a review from mfixstsci October 19, 2023 19:44
@bsunnquist
Copy link
Collaborator Author

bsunnquist commented Oct 19, 2023

This is ready for review. I've also incorporated the NIRCam Background Monitor as part of this PR, as it uses the data from the Claw databases. The following show the new NIRCam claw/background monitor pages:
Screen Shot 2023-10-19 at 2 39 18 PM
Screen Shot 2023-10-19 at 2 38 56 PM

@bsunnquist bsunnquist requested a review from bhilbert4 October 19, 2023 19:56
@bsunnquist
Copy link
Collaborator Author

@bhilbert4 @mfixstsci Just a heads up, I put in a few statements for testing purposes (see todo comments) so you can test it locally in a reasonable runtime. Once you're done reviewing and think it's ok, let me know, and I'll take these testing statements out before you merge it into production.

@mfixstsci
Copy link
Collaborator

Sounds great @bsunnquist I was able to run the monitor/generate outputs but I cant seem to get them to show up in the application.

@york-stsci
Copy link
Collaborator

Looking at it, the reason the outputs aren't showing up is that, when generated, they're being saved to the configured "outputs" directory, but then when loaded they're being looked for in /static, which is not the same place. The monitor should be looking for the images in the same place that it created them (and using the same method to generate the path). This will mean that images need to be copied between servers, until we configure an output directory to be put on central storage.

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.

@bsunnquist Just some housekeeping items and if you could address the comments from pep8speaks, I am happy with how this is looking. Maybe if @bhilbert4 wanted to look over claw_monitor.py it might be more useful than my general review since he has more experience working with NIRCam. I think we should also wait until @york-stsci has his relative path changes for static files in as well so we can make sure to implement them here and test. Thank you @bsunnquist!

jwql/website/apps/jwql/monitor_views.py Outdated Show resolved Hide resolved
jwql/website/apps/jwql/monitor_views.py Outdated Show resolved Hide resolved
jwql/instrument_monitors/nircam_monitors/claw_monitor.py Outdated Show resolved Hide resolved
jwql/instrument_monitors/nircam_monitors/claw_monitor.py Outdated Show resolved Hide resolved
jwql/instrument_monitors/nircam_monitors/claw_monitor.py Outdated Show resolved Hide resolved
jwql/instrument_monitors/nircam_monitors/claw_monitor.py Outdated Show resolved Hide resolved
jwql/instrument_monitors/nircam_monitors/claw_monitor.py Outdated Show resolved Hide resolved
@york-stsci
Copy link
Collaborator

If you want to be future-proof for the move to central storage, the current plan is that:

  • Any files that the monitor creates that are then displayed by the website will still go in the json.config['output'] directory (or, rather, a subdirectory for your own monitor, as happens now)
  • Any interim files that you create while working, and might want to keep around to make it easier to generate future outputs will be going in a json.config['working'] directory tree (which will be laid out as the "output" tree is in terms of relative structure).

The overall intent is that we will then create "output" directories in the JWQL area of central storage, and change the configuration file so that "working" points to where "output" does now, and "output" points to central storage. If you want to both work now and keep working after the transition, you can, in addition to creating the current output directory, create a working directory with pseudo-code like:

output_dir = config['output']
working_dir = config.get('output', working_dir)

So that if we haven't yet added the working directory keyword, it will default to using the current structure.

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.

Looks good to me. I haven't looked at the actual figures yet, but I'm assuming those are fine.

I believe NIRISS has marked the addition of a background monitor as high priority. In that case, future work on this might include stripping out the background functionality into its own class. Anyway, that's for down the road, and probably could be assigned to a NIRISS developer. This looks good as is, modulo the comments from Mees.

@york-stsci
Copy link
Collaborator

One other piece of housekeeping – there are at least two open issues on implementing the claw monitor. We probably want to look to see what issues are closed by this PR, and explicitly add them to the PR so that the issues will go away.

@mfixstsci
Copy link
Collaborator

One other piece of housekeeping – there are at least two open issues on implementing the claw monitor. We probably want to look to see what issues are closed by this PR, and explicitly add them to the PR so that the issues will go away.

Searching the issues this PR closes #1063 and closes #1315 @york-stsci

@mfixstsci
Copy link
Collaborator

Looks good to me. I haven't looked at the actual figures yet, but I'm assuming those are fine.

I believe NIRISS has marked the addition of a background monitor as high priority. In that case, future work on this might include stripping out the background functionality into its own class. Anyway, that's for down the road, and probably could be assigned to a NIRISS developer. This looks good as is, modulo the comments from Mees.

Thank you @bhilbert4 I would say that we should get this in as a first pass and then we can move the background monitor to the common_monitors in a separate PR.

@bsunnquist
Copy link
Collaborator Author

@mfixstsci @bhilbert4 @york-stsci Thanks all for the reviews - I think I've addressed everything. I'm now both saving and displaying the images using the same path, i.e. using get_config()['outputs'] ....

If you think this looks ok, let me know and I'll remove those few todo testing statements before you merge.

@mfixstsci
Copy link
Collaborator

@bsunnquist We just merged in the updates to the paths we store in the databases for png files. We are ready to revisit this now and hope to have it in with our next release this week.

@bsunnquist
Copy link
Collaborator Author

@mfixstsci sounds good - do you want me to remove those few testing statements now?

@mfixstsci
Copy link
Collaborator

I would say so. I am going to make a push really quick that updates the static path/where the app is pulling files from on the servers and using our generic user accounts. Pull those down first, make your changes, push them back up. At that point I'll pull them over onto the develop server to make sure everything is looking good there and we should be able to merge!

@mfixstsci mfixstsci changed the title [WIP] NIRCam Claw Monitor NIRCam Claw Monitor Dec 18, 2023
@mfixstsci mfixstsci merged commit a6d6079 into spacetelescope:develop Dec 18, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement NIRCam claw monitor
5 participants