replace OpticalSystem with AbstractOpticalSystem

[BrianGun]

Pull Request Template

Description

OpticalSystem is an abstract type. Standard Julia coding style is for all abstract types to begin with Abstract
Fixes #125

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

ran unit tests locally

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Test A
• Test B

Test Configuration(s):

• Firmware version:
• Hardware:
• Toolchain:
• SDK:

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[codecov-commenter]

Codecov Report

Merging #126 (942fa76) into main (17f1575) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main     #126   +/-   ##
=======================================
  Coverage   48.37%   48.37%           
=======================================
  Files          65       65           
  Lines        6659     6659           
=======================================
  Hits         3221     3221           
  Misses       3438     3438           

Impacted Files	Coverage Δ
src/Optical/OpticalSystem.jl	35.89% <ø> (ø)
src/Vis/Visualization.jl	5.75% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17f1575...942fa76. Read the comment docs.

[friggog]

I think there are more cases of this (abstract type names not following naming Abstract*) across the codebase, one example being OpticalInterface - probably worth fixing all together in one PR?

[alfredclwong]

I'm not sure that all abstract types should be named as Abstract*. In base Julia, for example, Number, Real and Integer are all abstract types. And this is what we have in OpticSim.

It would be good to establish a convention, though.

42 results - 20 files

src\Geometry\BoundingVolumeHierarchy.jl:
  15: abstract type BVHNode{T<:Real,S<:Primitive{T}} end

src\Geometry\Ray.jl:
  5: abstract type AbstractRay{T<:Real,N} end

src\Geometry\Surface.jl:
  17: abstract type Primitive{T<:Real} end
  57: abstract type Surface{T<:Real} <: Primitive{T} end
  80: abstract type ParametricSurface{S<:Real,N} <: Surface{S} end

src\Geometry\CSG\CSG.jl:
  6: abstract type CSGTree{T} <: Primitive{T} end

src\Geometry\CSG\Intersection.jl:
   8: abstract type IntervalPoint{T<:Real} end
  11: abstract type FinitePoint{T} <: IntervalPoint{T} end
  38:     interface::AllOpticalInterfaces{T} # returns a union of all OpticalInterface subtypes - can't have an abstract type here as it results in allocations

src\Geometry\CSG\Interval.jl:
  5: abstract type AbstractRayInterval{T<:Real} end

src\Geometry\Primitives\Curves\Spline.jl:
   8: abstract type CurveType end
   9: abstract type Rational <: CurveType end
  10: abstract type Euclidean <: CurveType end
  25: abstract type Spline{P<:CurveType,S<:Number,N,M} end
  33: abstract type SplineSurface{P<:CurveType,S,N,M} <: ParametricSurface{S,N} end

src\Geometry\Primitives\NonCSG\Stop.jl:
   5: abstract type StopShape end
   9: abstract type CircularStopShape <: StopShape end
  13: abstract type RectangularStopShape <: StopShape end
  20: abstract type StopSurface{T} <: Surface{T} end

src\GlassCat\GlassTypes.jl:
  30: abstract type AbstractGlass end

src\Optical\Emitter.jl:
    5: abstract type EmissionShape end
    6: abstract type EllipseEmission <: EmissionShape end
    7: abstract type RectEmission <: EmissionShape end
    8: abstract type PointEmission <: EmissionShape end
   15: abstract type RayOriginGenerator{T<:Real} end
   17: abstract type AbstractRayGenerator{T<:Real} end
   24: abstract type GeometricRayGenerator{T,O<:RayOriginGenerator{T}} <: AbstractRayGenerator{T} end
   31: abstract type OpticalRayGenerator{T} <: AbstractRayGenerator{T} end
  626: abstract type OpticalSourceGroup{T} <: OpticalRayGenerator{T} end

src\Optical\Grating.jl:
  12: abstract type WrapperSurface{T,S<:Surface{T}} <: Surface{T} end

src\Optical\Lattice.jl:
  5: abstract type Lattice{T<:Real} end
  6: abstract type InfiniteLattice{T<:Real} <: Lattice{T} end
  7: abstract type FiniteLattice{T<:Real,L<:InfiniteLattice{T}} <: Lattice{T} end

src\Optical\LensAssembly.jl:
  36: abstract type LensAssembly{T<:Real} end

src\Optical\OpticalInterface.jl:
   24: abstract type OpticalInterface{T<:Real} end
  296: # a few things need a concrete type to prevent allocations resulting from the ambiguities introduce by an abstract type (i.e. OpticalInterface{T})

src\Optical\OpticalSystem.jl:
  10: abstract type AbstractOpticalSystem{T<:Real} end

src\Optical\Emitters\AngularPower.jl:
  13: abstract type AbstractAngularPowerDistribution{T<:Real} end

src\Optical\Emitters\Directions.jl:
  13: abstract type AbstractDirectionDistribution{T<:Real} end

src\Optical\Emitters\Origins.jl:
  14: abstract type AbstractOriginDistribution{T<:Real} end

src\Optical\Emitters\Sources.jl:
  18: abstract type AbstractSource{T} <: OpticSim.OpticalRayGenerator{T} end

src\Optical\Emitters\Spectrum.jl:
  16: abstract type AbstractSpectrum{T<:Real} end

[BrianGun]

I gave the wrong justification. The correct justification is when the abstract type and its subtypes have common substrings. When they do the abstract type is prefixed with Abstract so it is easier to distinguish the types. At least this seems to be the pattern in the Julia core libararies.

We don't need to prefix all our abstract types with Abstract, just the ones that fit this pattern.

Number does not share a substring with Complex or Real so it stays Number.

By contrast, AbstractFloat shares a substring with BigFloat, Float64, etc., so it's prefixed Abstract.

Similarly, AbstractVector shares a substring with SVector and Vector so it's prefixed Abstract.

OpticalSystem shares a substring with CSGOpticalSystem and AxisymmetricOpticalSystem so it seems consistent to prefix it with Abstract.

[BrianGun]

@alfredclwong @galran @friggog if you agree with the logic of my last comment could one of you approve this PR?

[friggog]

We should try and follow this pattern elsewhere too (if there are other cases) - or consider renaming other types to improve consistency. I feel like a lot of stuff was named somewhat randomly to start with (with me being the primary culprit) and hasn't been renamed since, now we are aiming for clean extensible code that others can use this should take higher priority. This is certainly a step in the right direction :)