Poor performance of ray generation in Emitters.jl because of abstract struct field #208
Conversation
… struct field
Fixes #207
Changed all Integer declarations to Int64 in Emitters code. Allocations for all ray generation functions are now 0 bytes. Speed is greatly increased.
Also removed functions HOE, reflgrating, grating from other_examples.jl because they had old non-functioning code.
Codecov Report
@@ Coverage Diff @@
## main #208 +/- ##
==========================================
+ Coverage 56.41% 56.58% +0.16%
==========================================
Files 67 67
Lines 6388 6366 -22
==========================================
- Hits 3604 3602 -2
+ Misses 2784 2764 -20
Continue to review full report at Codecov.
|
Project.toml
Outdated
| @@ -5,6 +5,7 @@ repository = "https://github.com/microsoft/OpticSim.jl" | |||
| version = "0.5.1" | |||
|
|
|||
| [deps] | |||
| BenchmarkTools = "6e4b80f9-dd63-53aa-95a3-0cdb28fa8baf" | |||
There was a problem hiding this comment.
I don't think BenchmarkTools should be a dependency of the package itself - it's already installed separately for tests
There was a problem hiding this comment.
See comment below.
| @@ -254,34 +254,6 @@ function fresnel(convex = true; kwargs...) | |||
| Vis.drawtracerays(sys; test = true, trackallrays = true, numdivisions = 30, kwargs...) | |||
| end | |||
|
|
|||
| function grating(; period = 1.0, θ = 0.0, λ = 0.55, kwargs...) | |||
There was a problem hiding this comment.
These removals seem unrelated to the PR - is this intentional? If so maybe should be a separate PR
There was a problem hiding this comment.
It was intentional. A separate PR would be better in an ideal world but because it was a deletion of code that didn't work anymore it was simpler to add it into a single PR.
test/TestData/TestData.jl
Outdated
|
|
||
| ### Emitters | ||
|
|
||
| function RectGridAllocations() |
There was a problem hiding this comment.
100% agree on adding a test for Emitter allocations to ensure this kind of regression doesn't happen again - but it should really go in Benchmarks.jl and be added to all_benchmarks which will then be tested as part of the allocations tests
test/testsets/Emitters.jl
Outdated
| @@ -6,6 +6,7 @@ using Test | |||
| using OpticSim | |||
| using OpticSim.Emitters | |||
| using OpticSim.Geometry | |||
| using BenchmarkTools | |||
There was a problem hiding this comment.
I don't think this is needed
There was a problem hiding this comment.
yeah, that was an accident. I put it in temporarily because it was much faster to verify that the fixes were working than running the test set.
This is a weakness of our current setup. There are many times when you need to run a quick test like this, when you are first debugging the code. If this is done in the test sets then it's a 5 minute cycle as opposed to a few seconds if BenchmarkTools is a dependency.
I'll remove the BenchmarkTools dependency for now.
… struct field
Fixes #207
removed BenchmarkTools dependency from OpticSim.
Description
Fixes # 207 Poor performance of ray generation in
Changed Integer to Int64 in struct field declarations in Emitters code. The previous declaration was causing allocations and slow execution.
Type of change
How Has This Been Tested?
ran unit tests
Checklist: