Add parameter_march scripts #144
Conversation
| type: number | ||
| minimum: 0 | ||
| maximum: 90 | ||
| filter: |
There was a problem hiding this comment.
I'm wondering on the merits of including filter as a parameter here, as opposed to simply always using the current filter (and relegating a filter change to a separate script as necessary). Changing a filter is a fairly big deal for LSSTCam (though less so for ComCam), so it feels like it maybe shouldn't be buried in a separate script; it should be its own script. I don't think the same applies to any of the other parameters here (the initial slew could also be a big deal, but since the script ratchets in between triplets I don't really see an alternative). Curious what @tribeiro and others think.
There was a problem hiding this comment.
I am not sure if there has been a resolution for this, but FWIW I think the ability to change the filter like this is already present in other scripts. We can just leave it blank if we don't want to change it?
There was a problem hiding this comment.
You are right that changing the filter for LSSTCam is big deal. I have to think about this, for now, I would suggest we leave it here.
doc/news/DM-45761.feature.rst
Outdated
| @@ -0,0 +1,4 @@ | |||
| - - Introduced a suite of scripts for taking sensitivity matrices and parameter marches with LSSTCam and LSSTComCam: | |||
There was a problem hiding this comment.
Instead of adding multiple entries in one file, create one individual file for each entry. For each additional file after the first one, append a number to the file name, e.g.;
- doc/news/DM-45761.feature.rst
- doc/news/DM-45761.feature.rst.1
- doc/news/DM-45761.feature.rst.2
There was a problem hiding this comment.
Although, it looks like the Towncrier tests are complaining now...
| type: number | ||
| minimum: 0 | ||
| maximum: 90 | ||
| filter: |
There was a problem hiding this comment.
You are right that changing the filter for LSSTCam is big deal. I have to think about this, for now, I would suggest we leave it here.
| f"does not match n_steps {config.n_steps}." | ||
| ) | ||
| else: | ||
| raise TypeError("rotation_sequence must be either a number or a list.") |
There was a problem hiding this comment.
Isn't this enforced by the schema already?
| raise NotImplementedError() | ||
|
|
||
| @abc.abstractmethod | ||
| def take_images(self) -> None: |
There was a problem hiding this comment.
Can't you make take_images be the same for both cameras using self.camera?
There was a problem hiding this comment.
well, for ComCam take images are triplets, while for LSSTCam take images is only taking on image. That's why I had separated. Additionally, in the case of ComCam we ping RA to start processing the data
There was a problem hiding this comment.
Is LSSTCam really taking only one image? I think the idea here would be to do intra/extra regardless no?
There was a problem hiding this comment.
Well, LSSTCam gets intra focal and extra focal images directly from the corners for free, so we don't need to move the focal plane, unless we are using the "full array mode"
|
|
||
| @property | ||
| @abc.abstractmethod | ||
| def tcs(self): |
There was a problem hiding this comment.
The tcs is the same for both cameras, no need to make it an abstract property.
| raise NotImplementedError() | ||
|
|
||
| @abc.abstractmethod | ||
| async def configure_tcs(self): |
There was a problem hiding this comment.
This should be the same for both cameras as well.
| self.instrument_name = "LSSTComCam" | ||
|
|
||
| @property | ||
| def tcs(self): |
There was a problem hiding this comment.
See how this is just the same method for ComCam and LSSTCam? It should be in the base class.
| metadata.instrument = self.get_instrument_name() | ||
| metadata.filter = self.get_instrument_filter() | ||
|
|
||
| async def take_images( |
There was a problem hiding this comment.
you should be able to implement this method in the base class.
There was a problem hiding this comment.
See comment above. Why would I not separate it, since ComCam will be taking triplets while LSSTCam takes only one image? Doesn't it make it cleaner as is?
gmegh
force-pushed
the
tickets/DM-45761
branch
from
feca611 to
efba0d8
Compare
gmegh
force-pushed
the
tickets/DM-45761
branch
2 times, most recently
from
107ab4c to
aca266b
Compare
gmegh
force-pushed
the
tickets/DM-45761
branch
from
e733412 to
374f420
Compare
gmegh
force-pushed
the
tickets/DM-45761
branch
from
374f420 to
f5b956e
Compare
gmegh
force-pushed
the
tickets/DM-45761
branch
from
e9c39dd to
6fa09fe
Compare
No description provided.