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 cmos #72

Merged
merged 11 commits into from
Apr 2, 2024
Merged

Update cmos #72

merged 11 commits into from
Apr 2, 2024

Conversation

KriSun95
Copy link
Collaborator

@KriSun95 KriSun95 commented Mar 29, 2024

A PR for FOXSI-4's GSE 🦊


First and foremost, thank you so much for opening a PR for this repository. Any contribution is greatly appreciated.

Note

To make this process as easy as possible to follow when looking over and discussing the PR, please respond to each field below.

The fields below should inform the reader of the purpose(s) for the PR without having to infer things from the code itself.

What is reason for this PR

  • Bug fix
  • New feature
  • Behavioural change
  • Other
    • ...

Describe why the PR is needed

CMOS does not have a way to display images that are validated for flight. Ways to address this will be proposed in this PR and supporting document(s) from CMOS instrument team.

Note: This PR should offer no justification for these changes, only show how they are able to be implemented in the time available. Justification for all changes should be given in the supporting documentation where each commit is subject to approval.

Please provide a description of the changes/additions being made to the package

The changes/additions are as follows:

  1. Dark frame images for dark frame subtraction is implement with dark frames suitable for flight CMOS settings.
    • See commit d44eee0 for changes in the code
    • See commit 51c8215 for moving correct flight dark frames to the correct place
  2. PC data is now displayed such that values below a threshold of 15 DN are set to 0 and those above or equal to 15 DN are set to 1
    • See commit 9bd1b24
  3. To avoid the large bright region in the CMOS QL images from affecting the colour scaling of the image, a QL mask is applied to set a block of pixels in this lower left corner to 0 for plotting
    • See commit f5cd6cf
  4. The photon ratio values shown in the GSE were changed to consider the percentage of pixels with values (dark frame subtracted) greater than or equal to 60 DN in two masked areas. On-center area (mask 1 file below) is a small square in the centre of the PC CMOS image and Off-center (mask 2 file below) is a larger square area (with height of full image but not width) that excludes the On-centre square.
    • See commit dd552d6
    • where On-centre was previously called the "whole photon ratio" in the GSE display
    • where Off-centre was previously called the "part photon ratio" in the GSE display
    • this change was rejected and reverted with commit 507220e
  5. The QL image for CMOS 1 is now plotted in a log-scale colour map and the QL image for CMOS 2 is plotted with a linear-scale colour map.
    • See commit 3479f1d
    • See supporting documentation for justification also for dynamically changing colour-map minimum and maximum
    • This change involved changing how the image is plotted using matplotlib. All images in the GSE are plotted with an RGBA array; however, log-scale cannot be applied (or at least not easily) to an RGB/RGBA array when plotting with matplotlib.pyplot.imshow, for example. Therefore, the green values in the RGBA array (the colour that actually holds the image data to plot) is isolated and plotted as "grey-scale" using a custom colour-bar that runs from black to green. This is a colour-bar in-keeping with the previous previous plots of CMOS and the other detector plots.

Are there any dangers in this change

Each commit has been tested as far as I could and it does not appear to introduce anything breaking.

Note, however, that testing was limited as no data is available from flight settings that has produced an image. Therefore, the changes were validated either by corroboration in calculations from the GSE code and the instrument team member's code using noise or sealed source data, or by just trying to ensure the code runs and acts as expected with the changes applied (i.e., checking nothing breaks from the changes).

Please refer to supporting document from the CMOS instrument team for justifications and rationale for all changes in this PR.

Kristopher Cooper and others added 7 commits March 27, 2024 17:36
…ound less than 15 are set to zero and new values-background greater than or equal to 15 are set to 1. Note, the threashold for the whole photon ratio shown in the GSE still has a threshold of 60.
…ht pixel region in the lower left corner of the images do not interfere with the colout scaling. The mask file has been added to the data directory, loaded in within the CMOSQLCollection, and applied in the CMOSQLWindow.
…btracted pixels in the center of FOV (mask1) and then the percentage above the same threshold surrounding the centre (mask 2). Therefore, two new masks have been added in the form of NPY files, the CMOSPCCollection has two new methods in the class to collect this as well as two methods to load in the masks, and CMOSWidget has changed the text in the ratio fields to be on-center and off-center.
…near colour map. Depending on the colour map, vmins and vmaxs are determined differently. One change is that the QL images are no longer plotted with an RGBA array but just a grey-scal array with a black-to-green colour map.
@r-lycopene
Copy link

Here is the documentation which shows you why these changes are needed.
[https://docs.google.com/document/d/1ewaViXh3P6FQFPyb29a00uEa7fMJ3sHQZjXy9f8Fpak]

@LinErinG
Copy link
Member

For the QL mask part, I would like to request that the mask be applied in a way with fewer changes. Since the mask is a simple one (masking out one box), I think we can hardcode the area we want to mask out and do the masking in one or two lines. I would rather do that than read in an additional file, in order to constitute a smaller change to the code. @KriSun95

Kristopher Cooper added 2 commits April 1, 2024 13:55
…file containing a CMOS QL mask in it, the function that loads the mask now creates the mask directly. The file is therefore deleted from the repo.
…frame subtracted pixels in the center of FOV (mask1) and then the percentage above the same threshold surrounding the centre (mask 2). Therefore, two new masks have been added in the form of NPY files, the CMOSPCCollection has two new methods in the class to collect this as well as two methods to load in the masks, and CMOSWidget has changed the text in the ratio fields to be on-center and off-center."

This reverts commit dd552d6.
@KriSun95
Copy link
Collaborator Author

KriSun95 commented Apr 1, 2024

For the QL mask part, I would like to request that the mask be applied in a way with fewer changes. Since the mask is a simple one (masking out one box), I think we can hardcode the area we want to mask out and do the masking in one or two lines. I would rather do that than read in an additional file, in order to constitute a smaller change to the code. @KriSun95

Commit 7f9012d addresses this comment. The file was removed and numpy is used directly to create the mask (the same code used to produce the file in the first place). This means that the mask code is more transparent and another file does not need to be introduced into the repository. The overall code structure is kept as to only produce the mask once at the start and allow numpy to handle the quick masking for plotting.

Additionally, commit 507220e reverts the changes made in commit dd552d6 as this requested edit to the GSE was not accepted. The bullet point in the top description will be struck through.

Kristopher Cooper added 2 commits April 1, 2024 16:12
…om black to white instead of black to green. This was made possible due to the code written for plotting the log-QL iamge.
…d from 15 to 60 DN. This maintains consistency with previous analysis/tests/etc and is more appropriate than 15 DN.
@KriSun95 KriSun95 merged commit 7f37a13 into foxsi:main Apr 2, 2024
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

3 participants