DM-44917: Add psf focal plane plotting #42
Conversation
mfisherlevine
force-pushed
the
tickets/DM-44917
branch
from
7dcbae6
to
936550f
Compare
mfisherlevine
force-pushed
the
tickets/DM-44917
branch
from
936550f
to
eeed7e3
Compare
| f"from cdb_lsstcomcamsim.visit1_quicklook WHERE visit_id in ({visitString}) " | ||
| "ORDER BY visit_id" | ||
| ) | ||
| data["seqNum"] = data["visit_id"] % 10000 |
There was a problem hiding this comment.
It looks like you're already expecting to be given DimensionRecords for all of these visits, and it'd be a lot less fragile to pull these out in the loop below as
row["seqNum"] = record.seq_num
There was a problem hiding this comment.
Agreed. We don't generally approve of people parsing the visit_id because the form depends directly on the visit system and that isn't being checked here. It works because consDB is using a single visit system.
There was a problem hiding this comment.
I totally agree (and this is Josh's code btw). I think the issue here is that the data can come out of consDB in any order (at least in theory), so either we need to build the table up by looping over the DimensionRecords, which is inefficient, but would mean we have the seqNum from the loop, or we need to get it from something in the table already, otherwise we don't actually know which DimensionRecord it came from. Really, that should just already be a column in the database though, and then this would be a non-issue (here and many other places), but we don't have the wide views yet and KT doesn't want to replicate things anywhere.
There was a problem hiding this comment.
Oh, I see what Jim means about the "loop below" - I missed that. OK, that's gross but makes sense. OK, I'll do that 👍
No description provided.