DM-22605: Update camera for as installed values #153

SimonKrughoff · 2019-12-18T18:29:05Z

This pull request serves to update several key pieces of camera information based on the as installed sensors:

Sensor names and serials have been updated to reflect the installed sensors
Amp level information for saturation, read noise, gain, and QE curves are added

This also includes an update to the camera geometry to correctly assemble sensor grids including a unit test that uses measured spot projector data to confirm the assembly for both sensor vendors.

mfisherlevine

Some comments about the values might be fair to consider outside the scope, but on the other hand, if they're active already then I think some of them might need tweaking. And having said that actually, we shouldn't be merging things that we wouldn't allow if active just because they're not yet, because if they ever will be we're just shooting our future selves. Hit me up for a chat if you want to discuss with words.

SConstruct

policy/lsstCam/R10.yaml

mfisherlevine · 2019-12-19T13:42:04Z

policy/lsstCam/R10.yaml

+      C14 : { gain : 0.958300, readNoise : 6.323832, saturation : 188266.328125 }
+      C15 : { gain : 0.964008, readNoise : 6.311374, saturation : 179974.562500 }
+      C16 : { gain : 0.966000, readNoise : 6.678541, saturation : 190406.250000 }
+      C17 : { gain : 0.000000, readNoise : 0.000000, saturation : 0.000000 }


All the C17 values are zero. That seems likely to cause problems. I get that this is likely a bad amp, and therefore the values might be undefined, but this seems a bad way of dealing with it. Off the top of my head, I'd probably do gain of 1, a readNoise of 100 and a saturation of 0.001, with the saturation level likely being the most functional of those.

mfisherlevine · 2019-12-19T13:43:23Z

policy/lsstCam/R10.yaml

+      C05 : { gain : 0.913768, readNoise : 7.457952, saturation : 209321.250000 }
+      C06 : { gain : 0.906671, readNoise : 6.933922, saturation : 205319.265625 }
+      C07 : { gain : 0.912890, readNoise : 6.244426, saturation : 205120.796875 }
+      C10 : { gain : 0.844099, readNoise : 6.202470, saturation : 137353.203125 }


This saturation value seems implausible. If these are now being used we need to be careful. Can you confirm if this is now actually used to set the level at which SAT bits are set, or if these are currently decorative/for other things, with the SAT level used in isr still coming from a more high-level place?

It appears that value will be used. Specifically here. I also don't know if that value is meant to be ADU or electrons.

Maybe reusing that for full well values is not the right thing to do. Unfortunately, the amp objects are not trivial to extend to add new attributes.

Oh, I know there are bits of the code where saturation is relevant, I meant I wasn't sure if that value actually propagated through to there. It used to be set in a single place (until quite recently, on the order of weeks ago), and so I wasn't sure if the code had been updated to make this a per-amplifier value.

I believe the code I linked to uses the per amp value. Do you disagree?

Hi @mfisherlevine which values are implausible? The saturations?
@SimonKrughoff If you read these from the fw in the pickle file, these are in electrons. If the DM experts believe there are problems with these values I will let Steve Ritz take a look and see what he thinks.

@bxin Yes, it was the saturations I meant.
@SimonKrughoff I'd have to actually run stuff to check that it's picked up by ISR properly. I can do that if you like though. Either way, I think it's legit to get this stuff in so that it can be used, even if it's not at present (and I'm not saying it's not, just that I haven't checked), so it's not a comment against merging this, just asking if you'd checked whether the values are picked up and used or not.

mfisherlevine · 2019-12-19T13:43:41Z

policy/lsstCam/R10.yaml

+      C05 : { gain : 0.935021, readNoise : 7.222823, saturation : 204248.843750 }
+      C06 : { gain : 0.942203, readNoise : 6.780437, saturation : 203891.296875 }
+      C07 : { gain : 0.950371, readNoise : 6.599936, saturation : 199214.656250 }
+      C10 : { gain : 0.908633, readNoise : 9.134522, saturation : 133038.125000 }


Again, very low saturation.

These appear to be consistent with the plot Steve Ritz made.

They do indeed look to be consistent with that plot, but they're just as implausible in graphical form, no? Given that this is a property of the silicon and the gain, and that the gain is nowhere near as different as the saturation value is, do you really feel like such a sharp drop for one amp is physically plausible?

Also, it's very odd that it's always the same amp. I suspect this is some sort of artifact of the data/processing, and isn't the true value, but I'd love to hear more from someone who actually knows how this was measured/that plot was made.

@bxin Where did these saturation levels come from? If from EOTest, I think that they are meaningless (they're the point at which the variance drops due to total charge delocalisation)

yes from EOTest.

mfisherlevine · 2019-12-19T13:44:03Z

policy/lsstCam/R10.yaml

+      C05 : { gain : 0.939575, readNoise : 6.882386, saturation : 210176.796875 }
+      C06 : { gain : 0.945924, readNoise : 6.381833, saturation : 207313.468750 }
+      C07 : { gain : 0.939708, readNoise : 5.870911, saturation : 205092.906250 }
+      C10 : { gain : 0.922850, readNoise : 6.051341, saturation : 128881.054688 }


Again, low saturation.

mfisherlevine · 2019-12-19T13:44:59Z

policy/lsstCam/R10.yaml

+      C11 : { gain : 0.949116, readNoise : 6.602373, saturation : 195133.875000 }
+      C12 : { gain : 0.947612, readNoise : 7.270852, saturation : 198097.000000 }
+      C13 : { gain : 0.947605, readNoise : 6.832442, saturation : 197303.546875 }
+      C14 : { gain : 0.933952, readNoise : 16.432356, saturation : 194102.609375 }


Very high noise, though I'm least concerned about this compared to other comments about values.

mfisherlevine · 2019-12-19T22:00:56Z

This just in: all those gains are garbage! Turns out all the 0.9ish ones are before the bias-shift gain-mitigation gain change, or so I was just told. I take it that these numbers came from a source that is quite old, right?

timj · 2020-01-22T23:22:52Z

Before this merges please ensure that the spurious merge commit (54e4c56) on the branch is removed. A normal rebase should deal with that.

SimonKrughoff · 2020-03-10T20:18:30Z

Before this merges please ensure that the spurious merge commit (54e4c56) on the branch is removed. A normal rebase should deal with that.

@timj I believe this has been fixed. Let me know if I'm misunderstanding.

timj · 2020-03-10T20:21:30Z

@SimonKrughoff yes, looks fine now.

mfisherlevine · 2020-03-10T20:52:18Z

tests/xtest_assemble.py

+        for root, did, expected in zip(self.roots, self.ids, self.expecteds):
+            butler = Butler(root)
+            raw = butler.get("raw", dataId=did)
+            assembled = task.assembleCcd(raw)


I'm sure that's right, I'm just surprised it's run-like method isn't called .run() 😱

mfisherlevine · 2020-03-10T20:53:21Z

tests/xtest_assemble.py

+            butler = Butler(root)
+            raw = butler.get("raw", dataId=did)
+            assembled = task.assembleCcd(raw)
+            count = numpy.sum(expected['expected'].read().array - assembled.getImage().array)


It's probably fine, but does this work with float-type images, or only ints? Should this be assertAlmostEqual instead?

I have manually constructed both the input images and the expected images, so I have pretty tight control and would like to keep the assertEqual since it's more stringent.

I haven't been able to track down why it's failing, but need to get other work done in the mean time.

SimonKrughoff force-pushed the tickets/DM-22605 branch from a0c726e to 40843ec Compare December 18, 2019 18:32

SimonKrughoff requested a review from mfisherlevine December 18, 2019 18:33

mfisherlevine reviewed Dec 19, 2019

View reviewed changes

SimonKrughoff force-pushed the tickets/DM-22605 branch from 53f5f52 to 751be17 Compare February 10, 2020 18:31

SimonKrughoff force-pushed the tickets/DM-22605 branch from 642593c to 3b2d3e4 Compare March 6, 2020 23:40

SimonKrughoff changed the title ~~DM-22605: Update camera for measured R10 values~~ DM-22605: Update camera for as installed values Mar 9, 2020

SimonKrughoff requested a review from mfisherlevine March 10, 2020 20:16

mfisherlevine approved these changes Mar 10, 2020

View reviewed changes

SimonKrughoff mentioned this pull request Mar 10, 2020

U/cslage/com cam gain #185

Closed

SimonKrughoff and others added 17 commits March 12, 2020 12:22

Modify R10 for installed values

0c8f9da

Ingest QE curves for the lsstCam

97a0476

Correct amp locations relative to LCA-13381

b97e7f9

Remove spurious line

4a74624

Update R01 electronics for as built

8dce0d9

update lsstcam with EOTest results: R11, total=3 rafts

1295ec7

update lsstcam with EOTest results: R22, total=4 rafts

8a9b816

update lsstcam with EOTest results: R20, total=5 rafts

29da87e

update lsstcam with EOTest results: R21, total = 6 rafts

e6396c0

update lsstcam with EOTest results: R30, total = 7 rafts

15f0582

Temporarily disable the assembly test

cee106e

I haven't been able to track down why it's failing, but need to get other work done in the mean time.

Add RTM-009 to the camera description for ts8

f2d8afd

update lsstcam with EOTest results: R12, total = 8 rafts

9ddedbf

update lsstcam with EOTest results: R02, total = 9 rafts

50fa8a2

update lsstcam with EOTest results: R31, total =10 rafts

dcaed04

update lsstcam with EOTest results: R34, total = 11 rafts

2f9c1f1

update lsstcam with EOTest results: R13, total = 12 rafts

a3c3ffd

bxin and others added 15 commits March 12, 2020 12:22

update lsstcam with EOTest results: R23, total = 13 rafts

a6e2d96

update lsstcam with EOTest results: R14, total =14 rafts

791b5e9

update lsstcam with EOTest results: R32, total = 15 rafts

157c2a3

update lsstcam with EOTest results: R24, total = 16 rafts

4d59c9d

update lsstcam with EOTest results: R41, total = 17 rafts

1886bf7

update lsstcam with EOTest results: R33, total = 18 rafts

22a21cf

update lsstcam with EOTest results: R43, total = 19 rafts

413a535

Add RTM-017 to the camera description

54b802f

Add RTM-018 to camera description

c2bc240

Remove Dev suffix from serial in RTM-009 description

808aed3

update lsstcam with EOTest results: R03, total = 20 rafts

dec1525

update lsstcam with EOTest results: R42, total = 21 rafts

90eef6d

Add data for new assembly tests

7c6aa4f

Update test to use new test data

8580932

Rename test so it is found at scons time

bccf4fb

SimonKrughoff force-pushed the tickets/DM-22605 branch from 3b2d3e4 to bccf4fb Compare March 12, 2020 17:33

SimonKrughoff merged commit a864e21 into master Mar 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-22605: Update camera for as installed values #153

DM-22605: Update camera for as installed values #153

SimonKrughoff commented Dec 18, 2019 •

edited

mfisherlevine left a comment

mfisherlevine Dec 19, 2019

SimonKrughoff Dec 19, 2019

mfisherlevine Dec 19, 2019

SimonKrughoff Dec 19, 2019 •

edited

SimonKrughoff Dec 19, 2019

mfisherlevine Dec 19, 2019

SimonKrughoff Dec 20, 2019

bxin Dec 21, 2019

mfisherlevine Jan 6, 2020

mfisherlevine Dec 19, 2019

bxin Dec 21, 2019 •

edited

mfisherlevine Jan 6, 2020

mfisherlevine Jan 6, 2020

RobertLuptonTheGood Jan 6, 2020

bxin Jan 6, 2020

mfisherlevine Dec 19, 2019

mfisherlevine Dec 19, 2019

mfisherlevine commented Dec 19, 2019

timj commented Jan 22, 2020 •

edited

SimonKrughoff commented Mar 10, 2020

timj commented Mar 10, 2020

mfisherlevine Mar 10, 2020

mfisherlevine Mar 10, 2020

SimonKrughoff Mar 10, 2020

DM-22605: Update camera for as installed values #153

DM-22605: Update camera for as installed values #153

Conversation

SimonKrughoff commented Dec 18, 2019 • edited

mfisherlevine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonKrughoff Dec 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bxin Dec 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfisherlevine commented Dec 19, 2019

timj commented Jan 22, 2020 • edited

SimonKrughoff commented Mar 10, 2020

timj commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonKrughoff commented Dec 18, 2019 •

edited

SimonKrughoff Dec 19, 2019 •

edited

bxin Dec 21, 2019 •

edited

timj commented Jan 22, 2020 •

edited