-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Telescope perturbations #304
Conversation
A couple potential upgrade thoughts for this PR: For now, I hard coded the config processing of angles to require radians. For instance: telescope:
file_name: LSST_r.yaml
perturb:
rotX:
M2: 1.e-3 # radians implicit
rotY:
LSSTCamera: 1.e-3 # radians implicit it might be nice to use the normal config angle processing, e.g., telescope:
file_name: LSST_r.yaml
perturb:
rotX:
M2: 1.2 arcmin
rotY:
LSSTCamera: 0.03 deg I suppose that's probably doable by inserting some For shifts and Zernike perturbation amplitudes, the units are implicitly meters. A stretch goal might be to also make this flexible, e.g.: telescope:
file_name: LSST_r.yaml
perturb:
shift:
M2: [0.0, 0.0, 1e-3] m
LSSTCamera: [0.0, 0.1, 0.0] mm telescope:
file_name: LSST_r.yaml
perturb:
Zernike:
idx: [4, 5]
val: [1.e-4, 3.e-4] mm That might properly require some GalSim dev, (I don't think we had a reason to process physical lengths before), or we could do it special for imsim telescope loading. |
imsim/telescope_loader.py
Outdated
for group in perturb: | ||
for ptype, pvals in group.items(): | ||
if ptype == 'shift': | ||
for optic, shift in pvals.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only work if shift is a literal float value. I think we probably want this to be something that gets parsed in the normal way for config items so people could set them to change randomly according to some config-specified prescription or be lookups from some file or whatever.
To make that work, just change this to:
for optic in pvals.keys():
shift = galsim.config.ParseValue(pvals, optic, base, float)
You would also need to add base
as a parameter to this function, but that's straightforward.
Likewise for the rest ofc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Is there a good default value for base
that'd make it convenient to still use load_telescope
outside of the config context, e.g.,
telescope = imsim.load_telescope("LSST_r.yaml", perturb={'shift':{'M2': [0.0, 0.0, 1e-6]}})
?
I see that if I pass base=None
to ParseValue
it'll default to base=config
. What's the use case for base != config
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think the ParseValue snippet above may only work for a single scalar float, not a list of 3 floats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case for base != config?
Normally config
is the current local config dict. E.g. here, it's pvals
, the dict we're working on at the moment. base
should always be the base dict, where things like the main rng stuff and eval_variables
, etc. can live. Having a default base=None
is ok, but I would have thought we'd always be calling this in the context of a config parsing step, so you'd always have a dict to pass it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think the ParseValue snippet above may only work for a single scalar float, not a list of 3 floats?
Yes. If you want to allow either lists or floats, they would have to be checked explicitly I guess. They aren't considered equivalent types in the config parsing. (Lists are allowed with value_type=list
, but it's considered a different thing than when value_type=float
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseValue(pvals, optic, base, list)
works, but I suppose this doesn't type check for a list-of-floats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think if you tell it to parse a list, it will just return the list as is, without iterating further to parse each item as a float.
This would be easy to implement with the ParseValue bit by setting the type (last argument) to galsim.Angle rather than float. The m vs mm, etc. units would be a littl eharder. We don't have any "distance" type currently in GalSim, although we could add that. |
c44a4e9
to
378b24f
Compare
After looking at this again with fresh eyes, I think I might have the proposed hierarchy backwards. For example, instead of: perturb:
shift:
M1: [1.e-3, 1.e-3, 0.0]
LSSTCamera: [0.0, 0.0, -1.e-3] it's probably a bit more natural to collect all perturbations of a given optic together: perturb:
M1:
shift: [1.e-3, 1.e-3, 0.0]
LSSTCamera:
shift: [0.0, 0.0, -1.e-3] One could also break apart that |
I like the proposed transposition. That feels more natural to me too.
It's not out-of-the-box enforceable, but it wouldn't be too easy to write a short helper function that would parse something as a 3-component list.
Then in the main function, you would have things like
|
Does this apply to the Zernike coefficients as well? I.e., is there a config mechanism to make random length-n lists and not just individual random values that we should make available? |
Only via I was mostly thinking for shift items and other physical perturbations that people would likely want to draw from some normal distribution with an expected mean and sigma to simulate the expected uncertainty in the telescope positioning. |
Yeah, probably not Zernike perturbations directly, but I could imagine randomly perturbing bending modes once those get implemented. |
d8752f5
to
e1d65b0
Compare
ea4b3bb
to
23f5760
Compare
@rmjarvis , I think this is ready for you to take a look again. |
This looks good from my perspective. Not all the parameters are parsed yet -- some of them still need to be bare floats, rather than e.g. a random value, but it's not clear to me that this is a problem. Probably let's just merge this and start using it, and we'll see if there are use cases where we want more customization in the way we define anything. It might be that this covers all the plausible use cases already. |
PR adds rigid body perturbations of telescope optics and surface figure perturbations as Zernike polynomials. Also restores functionality where intra/extra focal wavefront sensors are automatically displaced.
I didn't attempt bending modes or gravitational surface figure errors here; that'll be next.