Fix new emitters

[alfredclwong]

[WIP]

I didn't get a chance to review PR #79 as I was off on holiday. Having a look at it now, several things slipped through the code review. These are the most obvious fixes - when I have more time, I may have a deeper look. There are also some issues from PR #70.

• We now have Emitter.jl, Emitter2.jl and Emitters.jl in the main branch. I don't agree with the decision to make dealing with this a low-priority matter, so I'll get this fixed here (delegated to Issue #104).
• Some type signatures need fixing. For example, in

  OpticSim.jl/src/Optical/Emitters.jl

  Lines 24 to 27 in 88a22f5

  	right(t::OpticSim.Geometry.Transform{T}) where {T<:Real} = normalize(Vec3(t[1,1], t[2,1], t[3,1]))
  	up(t::OpticSim.Geometry.Transform{T}) where {T<:Real} = normalize(Vec3(t[1,2], t[2,2], t[3,2]))
  	forward(t::OpticSim.Geometry.Transform{T}) where {T<:Real} = normalize(Vec3(t[1,3], t[2,3], t[3,3]))
  	OpticSim.origin(t::OpticSim.Geometry.Transform{T}) where {T<:Real} = Vec3(t[1,4], t[2,4], t[3,4])

  the parametric type T is unused, and in

  OpticSim.jl/src/Optical/Emitters.jl

  Line 222 in 88a22f5

  Base.length(d::Constant{T}) where {T} = 1

  and a few other cases, it's entirely redundant.
• Several relative import statements don't seem correct. For example,

  OpticSim.jl/src/Optical/Emitters.jl

  Lines 36 to 40 in 88a22f5

  	using ..OpticSim
  	using DataFrames
  	using Distributions
  	using ..Emitters

  doesn't make sense to me, as OpticSim and Emitters should be on different levels of nesting.
• Stylistically, Vec3/Vec4 may need some discussion. Since we're working with computer graphics, I'm not wholly opposed to explicit Vec3/Vec4 types, but some of the implementations, e.g.

  OpticSim.jl/src/Geometry/Transform.jl

  Line 44 in 88a22f5

  one3(::Type{T} = Float64) where {T<:Real} = Vec3{T}(one(T), one(T), one(T))

  are redundant due to LinearAlgebra (here, ones(Vec3{T}) would suffice). I think there are also some instances where the type signatures could make better use of multiple dispatch.
• Spelling/grammar mistakes.
• In https://microsoft.github.io/OpticSim.jl/dev/csg/, csg_ex.png is broken due to rotationd not being in scope. I'll change the doc build Github Action to fail on things like this now (I wasn't doing this because of the zoom lens example, but I'll comment that out).
• Docstring errors (delegated to Issue #103).
• What is this struct for? @galran

  OpticSim.jl/src/Optical/Emitters.jl

  Lines 11 to 13 in 88a22f5

  	struct MissingImplementationException <: Exception
  	msg::String
  	end

• Emitters testset (delegated to Issue #104).

Further issues:

• Redefinition of const parametric type T. @galran what's the intended purpose here?

  OpticSim.jl/src/Optical/Emitters.jl

  Line 120 in 103c70f

  T = eltype(power)

• export Geometry called from Emitters module. This is exporting an imported module.

  OpticSim.jl/src/Optical/Emitters.jl

  Line 1082 in 103c70f

  export Geometry, Spectrum, Directions, Origins, AngularPower, Sources

• export generate called from Directions and Origins submodules.

  OpticSim.jl/src/Optical/Emitters.jl

  Line 188 in 103c70f

  export generate

  OpticSim.jl/src/Optical/Emitters.jl

  Line 408 in 103c70f

  export generate

• Looking at these lines again, it seems that they should actually reside in Transform.jl. @galran is OpticSim.origin extended explicitly here because some previous code uses it?

  OpticSim.jl/src/Optical/Emitters.jl

  Lines 24 to 27 in 88a22f5

  	right(t::OpticSim.Geometry.Transform{T}) where {T<:Real} = normalize(Vec3(t[1,1], t[2,1], t[3,1]))
  	up(t::OpticSim.Geometry.Transform{T}) where {T<:Real} = normalize(Vec3(t[1,2], t[2,2], t[3,2]))
  	forward(t::OpticSim.Geometry.Transform{T}) where {T<:Real} = normalize(Vec3(t[1,3], t[2,3], t[3,3]))
  	OpticSim.origin(t::OpticSim.Geometry.Transform{T}) where {T<:Real} = Vec3(t[1,4], t[2,4], t[3,4])

[BrianGun]

The deep nesting of modules still seems unnecessarily complicated to me but in the interests of making progress let's try this out and see how it works.

[alfredclwong]

@galran I've now set the documentation build to fail on strict requirements, so we'll need to fix all the missing docstrings. This is the current list:

┌ Error: 15 docstrings not included in the manual:
│ 
│     OpticSim.Geometry.local2world :: Union{Tuple{StaticArrays.SMatrix{4, 4, T, 16}}, Tuple{T}} where T<:Real
│     OpticSim.Geometry.decomposeRTS :: Union{Tuple{StaticArrays.SMatrix{4, 4, T, 16}}, Tuple{T}} where T<:Real
│     OpticSim.GlassCat.CARGILLE.OG0607
│     OpticSim.Geometry.world2local :: Union{Tuple{StaticArrays.SMatrix{4, 4, T, 16}}, Tuple{T}} where T<:Real
│     OpticSim.NotebooksUtils.SetBackend :: Tuple{OpticSim.NotebooksUtils.Defs, String}
│     OpticSim.Emitters.Spectrum.spectrumpower :: Union{Tuple{T}, Tuple{OpticSim.Emitters.Spectrum.Measured{T}, T}} where T<:Real
│     OpticSim.GlassCat.CARGILLE.OG0608
│     OpticSim.NotebooksUtils.run :: Tuple{}
│     OpticSim.NotebooksUtils.run :: Tuple{Any}
│     OpticSim.NotebooksUtils.InitNotebook :: Tuple{}
│     OpticSim.Geometry.scale :: Tuple{T} where T<:Real
│     OpticSim.Geometry.scale :: Union{Tuple{T}, Tuple{T, T, T}} where T<:Real
│     OpticSim.Geometry.scale :: Union{Tuple{OpticSim.Geometry.Vec3{T}}, Tuple{T}} where T<:Real
│     OpticSim.GlassCat.CARGILLE.OG081160
│     OpticSim.NotebooksUtils.run_sample :: Union{Tuple{String}, Tuple{String, Bool}}
│ 
│ 
│ These are docstrings in the checked modules (configured with the modules keyword)
│ that are not included in @docs or @autodocs blocks.
└ @ Documenter.DocChecks ~/.julia/packages/Documenter/bFHi4/src/DocChecks.jl:69

Could you please add references to these into the appropriate .md files in docs? Thanks.

@BrianGun, the CARGILLE docstrings are a bit awkward. I think our two best options are either (1) to include a documentation page for CARGILLE, or (2) to remove CARGILLE entirely - currently it's only used once, in a testset.

[friggog]

@alfredclwong if you are no longer providing docs for all glasscat catalogs (which seems like a sensible choice) then it’s probably best to just remove the docstrings from cargille, there shouldn’t be any need to remove the code

Also updating the complete reference page in the docs with new sub modules (using auto docs) will remove these warnings if you don’t want dedicated docs pages for all these

[alfredclwong]

@alfredclwong if you are no longer providing docs for all glasscat catalogs (which seems like a sensible choice) then it’s probably best to just remove the docstrings from cargille, there shouldn’t be any need to remove the code

Also updating the complete reference page in the docs with new sub modules (using auto docs) will remove these warnings if you don’t want dedicated docs pages for all these

Removing the docstrings seems like the way to go. Or at least making them into multi-line comments. @BrianGun, do you have any objection to this? There is a tiny bit of convenience in seeing the glass info when you hover over a glass in the source code.

autodocs also sounds good. @galran, did you want to put those docstrings in new_emitters.md at all? If not, I'll just add them to ref.md.

[BrianGun]

@alfredclwong if you are no longer providing docs for all glasscat catalogs (which seems like a sensible choice) then it’s probably best to just remove the docstrings from cargille, there shouldn’t be any need to remove the code
Also updating the complete reference page in the docs with new sub modules (using auto docs) will remove these warnings if you don’t want dedicated docs pages for all these

Removing the docstrings seems like the way to go. Or at least making them into multi-line comments. @BrianGun, do you have any objection to this? There is a tiny bit of convenience in seeing the glass info when you hover over a glass in the source code.

autodocs also sounds good. @galran, did you want to put those docstrings in new_emitters.md at all? If not, I'll just add them to ref.md.

I don't have any objection to removing the docstrings.

[BrianGun]

@alfredclwong documentation check is failing.

[alfredclwong]

In 1916e2e, I'm not sure if Int or Integer is preferable. I chose Integer because it's more generic, but it might decrease performance because of this. @galran @BrianGun @friggog any thoughts?

[friggog]

@alfredclwong it might be a problem, Integer is an abstract type (supertype of all ints) so may not have known size to the compiler, possibly resulting in allocations. Int is an alias for Int32 or Int64 depending on the system, so is always of known size to the compiler. It might be ok, but make sure you check carefully whether any runtime allocations have been introduced

[alfredclwong]

@alfredclwong it might be a problem, Integer is an abstract type (supertype of all ints) so may not have known size to the compiler, possibly resulting in allocations. Int is an alias for Int32 or Int64 depending on the system, so is always of known size to the compiler. It might be ok, but make sure you check carefully whether any runtime allocations have been introduced

That's what I'm worried about. I feel like having abstract types in function signatures is OK, but there might be performance issues if we allow struct fields to be too generic. What's the best way to test for allocations - adding representative workloads to Benchmarks.jl?

[alfredclwong]

@galran do you have any recommendations for a representative workload for the Emitters code?

If you could add these to Benchmarks.jl, it'd be very helpful.

[BrianGun]

There are several things to consider. First what is the intent of the type? Here are the subtypes of Integer:

Bool
Signed <- ( BigInt, Int128, Int16, Int32, Int64, Int8) where <- means the elements on the right are subtypes
Unsigned <- (UInt128, UInt16, UInt32, UInt64, UInt8)

Will this code even work properly when passed all of these subtypes? Does it make sense to have this degree of generality? If the answer is no then it shouldn't be Integer.

Then there is the performance consideration. In general giving struct fields abstract types will lead to poor performance for the reasons @friggog mentioned. In this particular case Int64 is probably a better idea than Integer. No performance hit, no allocation, and plenty big enough to represent enough rays to keep a computer busy for a long time (billions of seconds, at least).

[alfredclwong]

Agreed, I'll revert the commit in a bit

[galran]

regarding the emitters, the only thing i can think of is running a couple of the examples (from the documentation or the notebook) - or at least just instantiating the non-visual part in order to not drag mesa into the mix.