SIM2D-109: Refactor code such that PfsConfigs are not dark/flat/science specific #4
Conversation
PaulPrice
force-pushed
the
tickets/SIM2D-109
branch
from
53e348f
to
6242a1d
Compare
I was using the pfsDesign to specify the spectra, even for arcs and darks, but the pfsDesign should only specify the top-end configuration, and the light source (sky vs quartz vs arc vs dark) should be separate. This allows us to dispense with the field definitions.
Not using it yet, but it might be useful down the road.
PaulPrice
force-pushed
the
tickets/SIM2D-109
branch
from
6242a1d
to
eed9d53
Compare
There was a problem hiding this comment.
A lot of changes do not seem to be related to making the code less dark/flat/science specific. But will wait for response from @PaulPrice .
| makeSim --detector $detector --pfiDesignId 3 --exptime 1 --noSky --expId 31 --imagetyp ARC | ||
| makeSim --detector $detector --pfiDesignId 4 --exptime 900 --expId 32 --imagetyp OBJECT --allOutput | ||
| makeSim --detector $detector --pfiDesignId 4 --exptime 900 --expId 33 --imagetyp OBJECT --allOutput | ||
| makeSim --detector $detector --pfiDesignId $pfiDesignId --exptime 0 --domeClosed --expId 0 --imagetyp BIAS |
There was a problem hiding this comment.
Is this change relevant for this particular ticket?
There was a problem hiding this comment.
I think it is not unreasonable: this ticket needs to clarify how to specify the source of the light going into the fibers. Making it harder to get "sky" along with your cals seems like a good thing.
But I'd ask whether it would be clearer to directly use the --imagetyp ? I.e. to have --imagetyp BIAS,DARK,FLAT,ARC imply --domeClosed?
There was a problem hiding this comment.
Because the PfsConfigs are no longer dark/flat/science specific, we only need two instead of the four we had previously. This change is why the ticket was done in the first place.
@CraigLoomis is right about tying things to --imagetyp; I have always viewed that as just manipulating the header, but it should be smarter.
| import pfs.instmodel.fieldDefinitions | ||
| from pfs.instmodel.slit import Slit | ||
| from pfs.instmodel.utils.generators import keepOdd, keepEven | ||
| from pfs.instmodel.makePfsConfig import makeScienceDesign | ||
|
|
||
| """Generate a PfiDesign""" |
There was a problem hiding this comment.
General remark: following DAMD-42 PfiDesign is now PfsDesign. But this can be addressed in a separate ticket.
There was a problem hiding this comment.
[Sorry for completely missing the fact that I was to review this]
Other than my one comment asking whether we could avoid ambiguities by leveraging the --imagetyp command line option I'm good with this. And frankly, replacing --imagetyp with something less low-level might be even better (--quartz, --arcs=NAMES, --bias, --dark=SS)
|
Ok following Craig's comments PR is approved (can no longer see option to mark this on GitHub as approved). So @PaulPrice feel free to merge. |
No description provided.