BP Map recipe template and test #168

juliamilton · 2024-08-22T16:37:37Z

Describe your changes

Wrote recipe template for bad pixel map generation
Implemented test_bp_map.py which tests the BP map generation on both simulated data and TVAC noise maps. Confirmed output with plots.
Updated elements of the Darks and Data to handle cases where image data is found in header[0], as it is in the TVAC files. Ran previous e2e tests to make sure these updates did not break those tests.
Updated the walker to handle the bp map, which doesn't provide input files, only a master dark and flat field

Type of change

New feature

Reference any relevant issues (don't forget the #)

Create recipe template for generating a BadPixelMap Calibration #149

Checklist before requesting a review

I have linted my code
I have verified that all unit tests pass in a clean environment and added new unit tests, as needed
I have verified that all docstrings are properly formatted and added new documentation, as needed

Linter output: (No data and header member outputs are false positives)

************* Module test_bp_map
tests/e2e_tests/test_bp_map.py:1:0: C0114: Missing module docstring (missing-module-docstring)
tests/e2e_tests/test_bp_map.py:15:0: R0914: Too many local variables (61/15) (too-many-locals)
tests/e2e_tests/test_bp_map.py:54:22: E1101: Instance of 'HDUList' has no 'header' member (no-member)
tests/e2e_tests/test_bp_map.py:56:22: E1101: Instance of 'HDUList' has no 'header' member (no-member)
tests/e2e_tests/test_bp_map.py:72:18: E1101: Instance of 'HDUList' has no 'data' member; maybe '_data'? (no-member)
tests/e2e_tests/test_bp_map.py:74:18: E1101: Instance of 'HDUList' has no 'data' member; maybe '_data'? (no-member)
tests/e2e_tests/test_bp_map.py:76:27: E1101: Instance of 'HDUList' has no 'data' member; maybe '_data'? (no-member)
tests/e2e_tests/test_bp_map.py:97:19: E1101: Instance of 'HDUList' has no 'data' member; maybe '_data'? (no-member)
tests/e2e_tests/test_bp_map.py:102:17: E1101: Instance of 'HDUList' has no 'header' member (no-member)
tests/e2e_tests/test_bp_map.py:103:17: E1101: Instance of 'HDUList' has no 'header' member (no-member)
tests/e2e_tests/test_bp_map.py:152:27: E1101: Instance of 'HDUList' has no 'data' member; maybe '_data'? (no-member)
tests/e2e_tests/test_bp_map.py:206:27: E1101: Instance of 'HDUList' has no 'data' member; maybe '_data'? (no-member)
tests/e2e_tests/test_bp_map.py:15:0: R0915: Too many statements (137/50) (too-many-statements)

Your code has been rated at 6.65/10 (previous run: 6.58/10, +0.06)

…map and figure out KGAIN

…changes with 12-to-l2 e2e

… test environment

juliamilton · 2024-08-22T17:27:45Z

Automated code checks are failing because it can't find the TVAC FITS files I use locally to run the test_bp_map.py e2e test.

semaphoreP · 2024-08-22T17:51:14Z

You should add the @pytest.mark.e2e decorator to your e2e test so pytest doesn't automatically run it as part of the automated testing (Automated testing not set up to run e2e tests)

semaphoreP

I have a few comments to simplify your implementation a bit. Also, have you tried running this code on the TVAC data from the Box folder? We want to make sure all the tests will run using just the files available there.

Also, it looks like you changed quite a few files that are out of scope of the BPMap recipe template and test. I would recommend not adding these to PRs in the future, since it just complicates the PR. If there are issues with other lines of code, please make a separate PR and issue about it.

corgidrp/darks.py

corgidrp/data.py

tests/test_data/simastrom/guesses.csv

tests/e2e_tests/test_bp_map.py

corgidrp/caldb.py

semaphoreP · 2024-08-23T12:01:19Z

tests/e2e_tests/test_bp_map.py

+    bp_map_outputdir = os.path.join(e2eoutput_path, "bp_map_output")
+    if not os.path.exists(bp_map_outputdir):
+        os.mkdir(bp_map_outputdir)
+    dark_current_filename = "dark_current_20240322.fits"


all these filenames should match the filenames on Box.

Echoing an above comment - there are issues with the files on Box. Once Marie is able to fix them I will update the names

Merged main into julia_bp_map and resolved conflicts.

…ata class

get_calib can take None as input

…ocstring

…e docstring

juliamilton · 2024-08-28T05:03:21Z

Merged changes that allow get_calib to take None as input. Addressed other comments above and separated tests into two different files (can put both functions in one file, whatever your preference is).

semaphoreP

Looks good. Thanks for promptly addressing those comments! I would prefer both tests in the same file, so if you can do that move, that'd be great.

There's also a tiny merge conflict in caldb.py that I think should be trivial (just keep the changes in main, since the change in indentation is intentional). It looks like this branch's version has a different line ending setting.

semaphoreP

Actually, sorry, I just tried running the tests, and the filepaths don't quite work with the Box data as the filenames aren't the same.

juliamilton · 2024-08-29T20:41:23Z

Actually, sorry, I just tried running the tests, and the filepaths don't quite work with the Box data as the filenames aren't the same.

Marie was going to update the noise map calibration files on Box, let me check with her on status of that.

mygouf · 2024-08-30T18:09:37Z

Actually, sorry, I just tried running the tests, and the filepaths don't quite work with the Box data as the filenames aren't the same.

Marie was going to update the noise map calibration files on Box, let me check with her on status of that.

That's right, yesterday afternoon didn't go as planned but I'm working on it right now. Thanks for your patience.

mygouf · 2024-08-30T18:24:43Z

Actually, sorry, I just tried running the tests, and the filepaths don't quite work with the Box data as the filenames aren't the same.

Marie was going to update the noise map calibration files on Box, let me check with her on status of that.

That's right, yesterday afternoon didn't go as planned but I'm working on it right now. Thanks for your patience.

Now done. Please note the changes to file names from

to

to be consistent with the TVAC file names. I'm not sure if that change breaks other parts of the code so this is something to keep an eye on.

juliamilton · 2024-08-30T19:36:24Z

One final item to note is that build_synthesized_dark takes in a dataset and looks for KGAIN in the headers. Since I am just using a dummy dataset of L1 frames for now (because BP map doesn't require any real input frames), I just set the KGAIN to a value since that parameter is not found in the header of L1 frames. In the future, however, will this mean that the frames need to be processed before passing them to build_synthesized_dark?

Merge 'main' into 'julia_bp_map' to update with latest changes

… read only...

semaphoreP · 2024-08-30T20:27:00Z

One final item to note is that build_synthesized_dark takes in a dataset and looks for KGAIN in the headers. Since I am just using a dummy dataset of L1 frames for now (because BP map doesn't require any real input frames), I just set the KGAIN to a value since that parameter is not found in the header of L1 frames. In the future, however, will this mean that the frames need to be processed before passing them to build_synthesized_dark?

Yes, I guess this is true, that the input dataset must have already gone through the convert_to_electrons. So, it cannot be a raw L1 frame.

semaphoreP

Ok, just a couple remaining small things, so that the tests can be picked out correctly when I run pytest in e2e mode.

semaphoreP · 2024-08-30T20:32:21Z

tests/e2e_tests/bp_map_e2e.py

+thisfile_dir = os.path.dirname(__file__)
+
+@pytest.mark.e2e
+def bp_map_master_dark_e2e(tvacdata_path, e2eoutput_path):


One last thing: based on how conftest.py collects tests for automated testing, the two tests here don't get identified unless the functions start with test_*. We just need to prepend "test_" to each function name.

semaphoreP · 2024-08-30T20:33:38Z

tests/e2e_tests/bp_map_e2e.py

+        output_path = os.path.join(bp_map_outputdir, "bp_map_master_dark_test.png")
+        plt.savefig(output_path)
+
+def bp_map_simulated_dark_e2e(tvacdata_path, e2eoutput_path):


In addition to preprending "test_", we also need the @pytest.mark.e2e decorator.

juliamilton added 10 commits August 17, 2024 13:16

build master dark not working when fits image data is in header 0

303afd8

code runs for TVAC inputs

15b8627

full test working with mock data. need to get representative TVAC bp …

54d6e24

…map and figure out KGAIN

reverted walker.py to only intake filelist

6254f3c

walker updated with master darks and flats in autogen recipe. tested …

3fc9b24

…changes with 12-to-l2 e2e

Both tests for BP map e2e passing

3d1673e

everything working except kgain

8da5507

Merge branch 'main' into julia_bp_map

00866cc

updated bp map to either 0 or 1, per the FDD

7fe98e0

updated formatting

f9cd028

juliamilton requested review from semaphoreP, mygouf and maxwellmb August 22, 2024 16:37

juliamilton linked an issue Aug 22, 2024 that may be closed by this pull request

Create recipe template for generating a BadPixelMap Calibration #149

Closed

3 tasks

juliamilton added 3 commits August 22, 2024 09:54

reverted to initial combined badpixels, which was already bitwise

ce4cefc

updated output path to be e2eoutput_path, which is a valid fixture in…

fc12506

… test environment

attempting to fix the pytest fixture issue with pytest.mark.parameterize

5ca19b2

added pytest.mark.e2e so that automated testing doesn't run e2e scripts

b30f580

semaphoreP requested changes Aug 23, 2024

View reviewed changes

juliamilton and others added 9 commits August 26, 2024 21:53

changes based on PR feedback

63f07c5

removing debugging statements

128edbf

removed extra test files

35ab0cb

reverted save function in data.py so that test_walker passes again

6411e3d

Revert darks.py and data.py to match main

dce9f33

Merge branch 'main' into julia_bp_map

613a5a1

Merged main into julia_bp_map and resolved conflicts.

get_calib can take None as input

93efab1

updating to use mock L1 input files instead of calibration files in d…

167e300

…ata class

updated get_calibs and walker.py

b6412aa

juliamilton and others added 5 commits August 27, 2024 20:55

Merge pull request #173 from roman-corgi/julia_bp_map_caldb_mod

647822e

get_calib can take None as input

both tests working

beeaa97

updated code post-PR change requests

5315a6c

fixed so it doesn't fail the linter for not having arguments in the d…

b94d11a

…ocstring

still failing the linter for some reason, just going to get rid of th…

3817eb9

…e docstring

juliamilton requested a review from semaphoreP August 28, 2024 04:59

semaphoreP approved these changes Aug 28, 2024

View reviewed changes

semaphoreP requested changes Aug 28, 2024

View reviewed changes

juliamilton added 2 commits August 29, 2024 13:33

Resolved merge conflicts

6e506c4

both BP map tests in one file

b28f8f5

no longer need to set kgain, removed

987538c

updated with new box file names

6341ae0

juliamilton added 2 commits August 30, 2024 12:43

Merge branch 'main' into julia_bp_map

d67c9ab

Merge 'main' into 'julia_bp_map' to update with latest changes

updated filepaths because read-only permission on Box was not in fact…

5225f4c

… read only...

semaphoreP reviewed Aug 30, 2024

View reviewed changes

juliamilton added 3 commits September 2, 2024 11:37

Merge branch 'main' into julia_bp_map

66e0550

added test_ to the beginning of test names

d0efcd6

fixed calls to functions

7973fa4

semaphoreP approved these changes Sep 3, 2024

View reviewed changes

semaphoreP merged commit 2bf4941 into main Sep 3, 2024
1 check passed

semaphoreP deleted the julia_bp_map branch September 3, 2024 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BP Map recipe template and test #168

BP Map recipe template and test #168

juliamilton commented Aug 22, 2024

juliamilton commented Aug 22, 2024

semaphoreP commented Aug 22, 2024

semaphoreP left a comment

semaphoreP Aug 23, 2024

juliamilton Aug 27, 2024

juliamilton commented Aug 28, 2024

semaphoreP left a comment

semaphoreP left a comment

juliamilton commented Aug 29, 2024

mygouf commented Aug 30, 2024

mygouf commented Aug 30, 2024 •

edited

Loading

juliamilton commented Aug 30, 2024

semaphoreP commented Aug 30, 2024

semaphoreP left a comment

semaphoreP Aug 30, 2024

juliamilton Sep 2, 2024

semaphoreP Aug 30, 2024

juliamilton Sep 2, 2024

BP Map recipe template and test #168

BP Map recipe template and test #168

Conversation

juliamilton commented Aug 22, 2024

Describe your changes

Type of change

Reference any relevant issues (don't forget the #)

Checklist before requesting a review

juliamilton commented Aug 22, 2024

semaphoreP commented Aug 22, 2024

semaphoreP left a comment

Choose a reason for hiding this comment

semaphoreP Aug 23, 2024

Choose a reason for hiding this comment

juliamilton Aug 27, 2024

Choose a reason for hiding this comment

juliamilton commented Aug 28, 2024

semaphoreP left a comment

Choose a reason for hiding this comment

semaphoreP left a comment

Choose a reason for hiding this comment

juliamilton commented Aug 29, 2024

mygouf commented Aug 30, 2024

mygouf commented Aug 30, 2024 • edited Loading

juliamilton commented Aug 30, 2024

semaphoreP commented Aug 30, 2024

semaphoreP left a comment

Choose a reason for hiding this comment

semaphoreP Aug 30, 2024

Choose a reason for hiding this comment

juliamilton Sep 2, 2024

Choose a reason for hiding this comment

semaphoreP Aug 30, 2024

Choose a reason for hiding this comment

juliamilton Sep 2, 2024

Choose a reason for hiding this comment

mygouf commented Aug 30, 2024 •

edited

Loading