Fix type validation and parsing in Parameter column #187
Conversation
Codecov Report
@@ Coverage Diff @@
## main #187 +/- ##
==========================================
- Coverage 56.65% 55.84% -0.81%
==========================================
Files 67 65 -2
Lines 6381 6328 -53
==========================================
- Hits 3615 3534 -81
- Misses 2766 2794 +28
Continue to review full report at Codecov.
|
src/Optical/OpticalSystem.jl
Outdated
| @@ -374,7 +374,12 @@ struct AxisymmetricOpticalSystem{T,C<:CSGOpticalSystem{T}} <: AbstractOpticalSys | |||
| elseif surface_type == "Aspheric" | |||
| semidiameter = convert(T, max(get_front_back_property(prescription, i, "SemiDiameter", zero(T))...)) | |||
There was a problem hiding this comment.
A call to convert shouldn't be necessary since semidiameter is declared of type T.
From the official documentation:
When is convert called?
The following language constructs call convert:
Assigning to an array converts to the array's element type.
Assigning to a field of an object converts to the declared type of the field.
Constructing an object with new converts to the object's declared field types.
Assigning to a variable with a declared type (e.g. local x::T) converts to that type.
A function with a declared return type converts its return value to that type.
Passing a value to ccall converts it to the corresponding argument type.
Similarly you should be able to replace this line:
vertices = convert(Vector{T}, -cumsum(replace(prescription[!, "Thickness"], Inf => 0, missing => 0)))
with
vertices::Vector{T} = -cumsum(replace(prescription[!, "Thickness"], Inf => 0, missing => 0))
There was a problem hiding this comment.
Agreed, this is much better. I was using convert(T, x) to replace several usages of T(x) which were being highlighted as "possible method call errors" by VSCode.
I'm adding a commit that makes better use of type declarations. This also helps with readability.
alfredclwong
force-pushed
the
fix/asos-parameters
branch
from
15814af to
e2cdf0d
Compare
| if firstelement && semidiameter !== NaN | ||
| if firstelement | ||
| systemsemidiameter = semidiameter | ||
| firstelement = false | ||
| end |
There was a problem hiding this comment.
This change will allow the Stop semidiameter to define the systemsemidiameter if it's the first element. This is different from the previous behaviour - is this OK?
There was a problem hiding this comment.
ask Joel Kollin. He could tell us if this makes sense optically.
There was a problem hiding this comment.
I asked the Cambridge team today and the general consensus was that individual semidiameters are important but not so much the system semidiameter. And, if anything, it'd be more useful to have this as the max value. I'll have a look around the code tomorrow to see if this is actually used anywhere currently.
Vector{<:Pair{<:AbstractString,<:Real}}which allows for flexibility beyond numerically indexed coefficients.Vector{Tuple{Int,S}}to conform with existing surface constructors (temporary fix, we'll probably rewrite the constructors soon).Currently (without these changes),
Examples.draw_zoomlensesdoesn't work.