Allow for surface type extensibility in AxisymmetricOpticalSystem #179
Conversation
alfredclwong
force-pushed
the
alfred/asos-surfaces
branch
from
53cf644 to
73e562b
Compare
alfredclwong
force-pushed
the
alfred/asos-surfaces
branch
from
73e562b to
de74a1f
Compare
This comment has been minimized.
This comment has been minimized.
alfredclwong
force-pushed
the
alfred/asos-surfaces
branch
from
5499131 to
019a50d
Compare
alfredclwong
changed the title
alfredclwong
force-pushed
the
alfred/asos-surfaces
branch
from
cd19524 to
d0027e1
Compare
| [4 => 1.0386E-04, 6 => 1.4209E-07, 8 => -8.8495E-09, 10 => 1.2477E-10, 12 => -1.0367E-12, 14 => 3.6556E-15], | ||
| [4 => 4.2721E-05, 6 => 1.2484E-07, 8 => 9.7079E-09, 10 => -1.8444E-10, 12 => 1.8644E-12, 14 => -7.7975E-15], | ||
| [4 => 1.1339E-04, 6 => 4.8165E-07, 8 => 1.8778E-08, 10 => -5.7571E-10, 12 => 8.9994E-12, 14 => -4.6768E-14], |
There was a problem hiding this comment.
Not quite a dictionary, but I think Pairs here are more expressive than tuples regarding our intended usage. This will easily convert to a Dict if needed later.
There was a problem hiding this comment.
Yeah I think there is a better way to do this still eg where surface type is the actual type name allowing you to get the constructor for the surface dynamically and then you could pass the parameter dict as keyword args. This is a pretty fundamental change and might mean modifying the surface constructors too (ughh) but would mean that new surface types would be supported directly and there wouldn’t be any need for hard coding stuff when setting up axisymmetric systems.
Anyway this seems reasonable for now, but I think there is a better solution there that might be easiest to achieve by starting almost from scratch on the design of AxisymmetricOpticalSystem
There was a problem hiding this comment.
I see what you mean - got the wrong idea earlier!
It'll definitely require a lot of changes, and it'll get mixed up with some other discussions we're having now about prescription DataFrames for non-axisymmetric systems... (although this also means it's a good time for a rewrite). Let's go with this for now - in the short term, all I want is to add support for Zernike surfaces. I'll factor this into our discussions and work towards something smoother in the future.
side note: i'd still like to stick with Vector{Pair} for coefficients
src/Examples/other_examples.jl
Outdated
| OptimizeRadius = [false, true, true, true, true, true, true, false], | ||
| # OptimizeRadius = [false, true, true, true, true, true, true, false], |
There was a problem hiding this comment.
These columns can be reintroduced later when the Optimization code is more mature. But they shouldn't be used within the AxisymmetricOpticalSystem constructor. Instead, we should have some kind of AxisymmetricOptimization constructor which parses the extra columns.
Project.toml
Outdated
| @@ -2,7 +2,7 @@ name = "OpticSim" | |||
| uuid = "24114763-4efb-45e7-af0e-cde916beb153" | |||
| authors = ["Brian Guenter", "Charlie Hewitt", "Ran Gal", "Alfred Wong"] | |||
| repository = "https://github.com/microsoft/OpticSim.jl" | |||
| version = "0.5.0" | |||
| version = "0.5.0-DEV" | |||
There was a problem hiding this comment.
We still need a bug fix from PR #183 before releasing v0.5.0.
Fixes #176
I'll add support for Zernike surfaces in another PR - thought it'd be best to keep this one focused on reworking the core framework first.
Changes
Surfacecolumn header toSurfaceTypeSurfaceTypecolumn (e.g.:Object->"Object")Parametercolumn to allow for surface type extensibility[TODO] Write tests for AxisymmetricOpticalSystem(will probably rework the system soon so not worth it at the moment)