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 overwrite option when generating preview images #1032

Merged
merged 4 commits into from
Sep 23, 2022

Conversation

bhilbert4
Copy link
Collaborator

This PR adds an overwrite option to generate_preview_images.py, which will allow existing preview images and thumbnail images to be re-created and overwritten in cases where they already exist. This will make it easier to regenerate preview images, since we won't have to go in and manually delete them prior to running the code. This will also eliminate the time where a particular file has no preview image, which is currently the case between the time when the old image is deleted and the new image is generated.

@bhilbert4 bhilbert4 self-assigned this Aug 30, 2022
@pep8speaks
Copy link

pep8speaks commented Aug 30, 2022

Hello @bhilbert4, Thank you for updating !

Line 50:1: E402 module level import not at top of file
Line 51:1: E402 module level import not at top of file
Line 52:1: E402 module level import not at top of file
Line 462:5: E303 too many blank lines (2)

Comment last updated at 2022-08-30 20:09:47 UTC

@bhilbert4 bhilbert4 changed the title [WIP]: Add overwrite option when generating preview images Add overwrite option when generating preview images Aug 30, 2022
@bhilbert4
Copy link
Collaborator Author

@mfixstsci @BradleySappington this is ready for review. I tested it on the test server and all looks well. I ran it on program 01068 both with and without the new overwrite flag and it worked as expected.

I also removed a strange line from preview_images.py that was causing a crash. I'm not sure how that line got in there (or maybe, how it was not removed in a prior PR), but it was referencing pixmap before it existed. I believe that line was meant to be replaced by the line below it (that uses self.dq rather than pixmap) in a previous PR. Either way, that whole function is re-written in #1024, which we also need to use when regenerating the preview images on ops.

pool = multiprocessing.Pool(processes=int(SETTINGS['cores']))
results = pool.map(process_program, program_list)
results = pool.starmap(process_program, program_list)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since each tuple will have the same second parameter in the list, you could use the program_list with map (instead of starmap), and just send overwrite as the 'y' parameter. This wouldn't change the functionality, just simplifies the code. This is fine as is, just giving feedback.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand. map() only accepts one parameter, right? So in that case I'd have to also make program_list a tuple, and I'd have to change the parameters of process_program() such that it accepts a single parameter that is then broken out into program id and overwrite status, right? Or am I misinterpreting the way that map() works?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I withdraw my comment, Map can take multiple parameters, but they both need to be iterable anyway, this way makes more sense.

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.

minor comments, nothing critical

jwql/jwql_monitors/generate_preview_images.py Show resolved Hide resolved
@mfixstsci mfixstsci merged commit 4988924 into spacetelescope:develop Sep 23, 2022
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.

None yet

4 participants