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

Fix radio buttons with e.g. o001 #996

Merged
merged 9 commits into from
Aug 18, 2022

Conversation

bhilbert4
Copy link
Collaborator

Resolves #990

This PR fixes the bug that was causing entries such as o001 to show up in the list of filename suffixes in the radio buttons for an individual file.

Previously, the suffix of the file was being found by assuming that it was the 4th section of the filename (separated by '_'). This doesn't work for crf files because they have an extra section that lists the association value, immediately before the 'crf' suffix. By using the filename parser, this should now work correctly for all file types.

@bhilbert4 bhilbert4 self-assigned this Jul 20, 2022
@bhilbert4 bhilbert4 added this to In progress in v1.2.0 via automation Jul 20, 2022
@pep8speaks
Copy link

pep8speaks commented Jul 20, 2022

Hello @bhilbert4, Thank you for updating !

Line 53:34: E126 continuation line over-indented for hanging indent
Line 217:64: E226 missing whitespace around arithmetic operator
Line 1116:17: W503 line break before binary operator
Line 1117:17: W503 line break before binary operator
Line 1118:17: W503 line break before binary operator
Line 1119:17: W503 line break before binary operator
Line 1539:9: E722 do not use bare 'except'

Comment last updated at 2022-08-17 16:32:18 UTC

@bhilbert4
Copy link
Collaborator Author

bhilbert4 commented Jul 20, 2022

  • Still needs to be tested on the test server.

@bhilbert4 bhilbert4 changed the title Fix radio buttons with e.g. o001 [WIP]: Fix radio buttons with e.g. o001 Jul 20, 2022
@bhilbert4 bhilbert4 changed the title [WIP]: Fix radio buttons with e.g. o001 Fix radio buttons with e.g. o001 Jul 22, 2022
@bhilbert4
Copy link
Collaborator Author

bhilbert4 commented Jul 22, 2022

Tested on the test server and it seems to be working. The radio button for crf files now includes the name of the association (e.g. 'o001_crf'). This is because crf filenames have an extra string when you break up a filename between underscores.

I also found and fixed the bug that was causing the slider bar under the image to always list the image as '1/NaN' integrations. There was a typo in the directory being searched for jpgs. I also tweaked the code a bit so that the value now used in place of the NaN is the the total number of integrations in the file, rather than the number of integrations for which we have created preview images. These numbers are only different for exposures that have more than 100 integrations, which means only TSO observations I think. But using the true number of integrations is much less confusing than the number of jpg files, since for observations with more than 100 integrations we start making preview image for every 100th integration, in order to save disk space. I also tested this on the test server and it looks good.

  • Check a crfints example
    Actually, I'm not sure where to find crfints data. That will only be made if someone runs the stage 3 pipeline and feeds it calints files. I'm not sure under what situation that would happen. Maybe only for moving target (i.e. solar system) data?

@bhilbert4 bhilbert4 linked an issue Jul 22, 2022 that may be closed by this pull request
@bhilbert4
Copy link
Collaborator Author

I just checked the Jupiter observations from commissioning and there are no crfints files in the filesystem. So maybe we assume this fix is ok for those too and fix it if we find a problem in the future?

If you agree @mfixstsci then this is ready for review.

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
Copy link
Collaborator

@bhilbert4 I went reviewed and fixed conflicts. Anything else before merging?

@bhilbert4
Copy link
Collaborator Author

@mfixstsci I think it's ready to go!

@mfixstsci mfixstsci merged commit ed3707b into spacetelescope:develop Aug 18, 2022
v1.2.0 automation moved this from In progress to Done Aug 18, 2022
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.

o001 radio button Make the integration slider bar smarter
3 participants