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 entry in filename_parser for segmented guider files #962

Merged

Conversation

bhilbert4
Copy link
Collaborator

Resolves #961
This PR adds code that allows the filename_parser to understand Guider segment files. These files are used in cases where the amount of Guider data is above some threshold, in which case it is split into multiple files.

@bhilbert4 bhilbert4 self-assigned this Jun 2, 2022
@pep8speaks
Copy link

pep8speaks commented Jun 2, 2022

Hello @bhilbert4, Thank you for updating !

Line 41:34: E126 continuation line over-indented for hanging indent
Line 456:21: E117 over-indented
Line 459:5: E303 too many blank lines (2)
Line 559:86: W504 line break after binary operator
Line 560:79: W504 line break after binary operator
Line 566:80: W504 line break after binary operator
Line 567:79: W504 line break after binary operator

Comment last updated at 2022-06-24 18:59:06 UTC

@bhilbert4
Copy link
Collaborator Author

Local test shows the parser working correctly. Just need to test on a server using the search field on the home page to see if any other changes are needed in support of the filenames.

@bhilbert4
Copy link
Collaborator Author

Looks like 1118 has lots of challenging files:
Provided file jw01118008001_01_msa.fits does not follow JWST naming conventions

@bhilbert4 bhilbert4 changed the title [WIP]: Add entry in filename_parser for segmented guider files Add entry in filename_parser for segmented guider files Jun 23, 2022
@bhilbert4
Copy link
Collaborator Author

I also added code to be able to parse filenames such as jw01118008001_01_msa.fits, which originally failed above. This is an MSA metadata file.

@bhilbert4
Copy link
Collaborator Author

@mfixstsci and/or @BradleySappington I think this is ready for review. I tested it on the dev server by attempting to retrieve program 1118 using the "Find a JWST Proposal or File" field on the main page. Previously this program was causing the website to crash because of the filename parser error. Now it loads successfully....with a message saying that it cannot return data from proposals that use more than 1 instrument.

@mfixstsci
Copy link
Collaborator

@mfixstsci and/or @BradleySappington I think this is ready for review. I tested it on the dev server by attempting to retrieve program 1118 using the "Find a JWST Proposal or File" field on the main page. Previously this program was causing the website to crash because of the filename parser error. Now it loads successfully....with a message saying that it cannot return data from proposals that use more than 1 instrument.

Just tried it myself and it works as expected. Do we know of any multi-instrument programs we can test?

@bhilbert4
Copy link
Collaborator Author

See #901. It looks at the moment like all programs are considered multi-instrument because they all use FGS for guide stars. That's a separate issue to this, and one we should try to complete before the next release. In the meantime, it might take some searching to find a single-instrument program that also has a segmented FGS file in it. And it won't be possible to find a program that has only nirspec and an MSA metadata file, since FGS will also be used in all of those.

jwql/utils/utils.py Outdated Show resolved Hide resolved
jwql/utils/utils.py Outdated Show resolved Hide resolved
@bhilbert4
Copy link
Collaborator Author

I also added tests for these two new possibilities in the filename_parser

@bhilbert4 bhilbert4 added this to In Review in v1.2.0 Jun 24, 2022
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.

Looks good @bhilbert4
Could potentially store all constants as a dictionary to make import easier, but that would also create more lines as you'd have to extract the values before putting into the regex to keep it from getting too muddled. A-OK to keep as is :)

@bhilbert4
Copy link
Collaborator Author

Looks good @bhilbert4 Could potentially store all constants as a dictionary to make import easier, but that would also create more lines as you'd have to extract the values before putting into the regex to keep it from getting too muddled. A-OK to keep as is :)

I agree. I started to implement this as a dictionary, but that made readability in filename_parser() even worse.

@BradleySappington
Copy link
Collaborator

BradleySappington commented Jun 24, 2022 via email

@mfixstsci
Copy link
Collaborator

Looks good @bhilbert4 Could potentially store all constants as a dictionary to make import easier, but that would also create more lines as you'd have to extract the values before putting into the regex to keep it from getting too muddled. A-OK to keep as is :)

Agreed, I think it could be done this way and not cost much, but I think that is something we could improve on if we need to import those constants anywhere else :)

@bhilbert4
Copy link
Collaborator Author

Woo hoo! Merging! Thanks for the review!

@bhilbert4 bhilbert4 merged commit f0398ea into spacetelescope:develop Jun 24, 2022
v1.2.0 automation moved this from In Review to Done Jun 24, 2022
@bhilbert4 bhilbert4 deleted the parse-fgs-segmented-filenames branch June 24, 2022 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Update filename parser to handle segment files of Guider data
4 participants