Skip to content
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

Support for tilted surfaces? #61

Open
dkirkby opened this issue Apr 7, 2019 · 5 comments
Open

Support for tilted surfaces? #61

dkirkby opened this issue Apr 7, 2019 · 5 comments

Comments

@dkirkby
Copy link
Contributor

dkirkby commented Apr 7, 2019

It looks like parse_coordSys() only supports (x,y,z) translations and not any rotations. Is that correct? (I am trying to implement the DESI ADC, which has 2 tilted surfaces).

@jmeyers314
Copy link
Owner

I think that's right. I probably hadn't run into a case yet where I needed to parse a rotated coordsys when I wrote parse_coordSys.

I think the API for enabling this exists though (rotateLocal()). The tricky part is probably just deciding what order rotations and shifts found in the yaml config should be applied (or how to support arbitrary ordering). My hunch is that shift and then rotate makes sense (since many use cases will want to continue along the optic axis before inserting a rotated part).

Then there's the question of how to represent an arbitrary rotation. For now though, would simply rotate around 'x' or rotate around 'y' be sufficient? I.e., something like

coordSys:
  z: 0.1 # meters
  rotX: 0.1 # radians, about the new local x axis

@dkirkby
Copy link
Contributor Author

dkirkby commented Apr 8, 2019

I just implemented a minimal change to parse_coordSys() that allows an optional rotation about one axis that is applied after any shifts in #62. I used degrees as the angular unit.

I think a more flexible and intuitive long-term solution is to apply coord transforms in the order that they are specified in the yaml file.

Currently this can be achieved by nesting CompoundOptics, each with their own shift and up to one rotation. A more elegant approach would be to have a single coordSys block with its transforms applied in order. However, since yaml parses into unordered dicts by default, this would require a change to the top-level parsing API, i.e.

config = yaml.safe_load(open(DESI_fn))
fiducial_telescope = batoid.parse.parse_optic(config['opticalSystem'])

becomes:

fiducial_telescope = batoid.parse.parse_optic(open(DESI_fn))

By delegating the yaml parsing to your code, you can always build OrderedDicts using this recipe.

@dkirkby
Copy link
Contributor Author

dkirkby commented May 9, 2019

The current implementation is somewhat limited since it only allows a single rotX,Y,Z at a time, combined with a possible shift. I currently combine them using, e.g.

coordSys = coordSys.shiftLocal(shift)
coordSys = coordSys.rotateLocal(RotY(angle))

I have only been using these with commuting combination of shift and rotate, e.g.

coordSys = {'z': 0.06015, 'rotY': 0.00430224}

However, I think non-commuting combinations will shift then rotate. Do you agree with this @jmeyers314?

@jmeyers314
Copy link
Owner

I believe this is the case. The best way I've discovered to sanity check questions like this is the introduce a noticeable shift+rotation and then 3D-plot the optics to see if the results make sense. That's the impetus for the HSC 3D notebook, for example.

@jmeyers314
Copy link
Owner

Actually, I think your example isn't actually commuting (at least with batoid conventions; sorry if these need better documentation). If I rotate around the y-axis, that changes the direction of the z-axis. So if I rotate-then-shift, I get a different local origin than if I shift-then-rotate:

>>> import batoid                                                                         
>>> sys = batoid.CoordSys()                                                               
>>> print(sys)                                                                            
CoordSys([0,0,0],[[1,0,0],[0,1,0],[0,0,1]])

>>> sys.shiftLocal([0,0,0.1])                                                             
CoordSys([0,0,0.1],[[1,0,0],[0,1,0],[0,0,1]])

>>> sys.shiftLocal([0,0,0.1]).rotateLocal(batoid.RotY(0.1))                               
CoordSys([0,0,0.1],[[0.995004,0,0.0998334],[0,1,0],[-0.0998334,0,0.995004]])

>>> sys.rotateLocal(batoid.RotY(0.1)).shiftLocal([0,0,0.1])                               
CoordSys([0.00998334,0,0.0995004],[[0.995004,0,0.0998334],[0,1,0],[-0.0998334,0,0.995004]])

(The orientation of the sys commutes, but the origin doesn't).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants