Convert parabola args to from cx to fx.#70
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR converts parabola surface parameters from legacy "cx, cy" format to the new "fx, fy" focal length format. The conversion applies the formula fx = 1/(2cx) and fy = 1/(2cy) to transform between the two parameter systems.
- Implemented parameter conversion for parabola surfaces from cx/cy to fx/fy format
- Extended performance test timeout to accommodate the changes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| coretrace/simulation_data/surface.cpp | Added conversion logic from cx/cy to fx/fy parameters for parabola surfaces |
| google-tests/regression-tests/native_runner_performance_test.cpp | Increased performance test timeout from 16 to 40 seconds |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| double fx = 1.0 / (2.0 * cx); | ||
| double fy = 1.0 / (2.0 * cy); | ||
| retval = make_surface<Parabola>(fx, fy); |
There was a problem hiding this comment.
Division by zero will occur if cx or cy is zero. Add validation to check that cx and cy are non-zero before performing the conversion.
| double fx = 1.0 / (2.0 * cx); | |
| double fy = 1.0 / (2.0 * cy); | |
| retval = make_surface<Parabola>(fx, fy); | |
| if (cx == 0.0 || cy == 0.0) | |
| retval = nullptr; | |
| else { | |
| double fx = 1.0 / (2.0 * cx); | |
| double fy = 1.0 / (2.0 * cy); | |
| retval = make_surface<Parabola>(fx, fy); | |
| } |
There was a problem hiding this comment.
The correct place to do this is probably to check for finite values in the Parabola constructor. That can be done later. Many of the surfaces and apertures also need similar checks.
|
|
||
| std::chrono::duration<double, std::milli> dur = t1 - t0; | ||
| EXPECT_TRUE(dur.count() < 16000.0); | ||
| EXPECT_TRUE(dur.count() < 40000.0); |
There was a problem hiding this comment.
How long does this take to run on your machine? It passes regularly on GitHub runners and my laptop with the smaller value.
There was a problem hiding this comment.
I reverted this change (I was previously using debug 😕)
jmaack24
left a comment
jmaack24
left a comment
There was a problem hiding this comment.
One of the unit tests also needs to be updated: https://github.com/NREL/SolTrace/blob/de4256a2ee2f3e5068c8171e7fa9c7d4172d4e92/google-tests/unit-tests/simulation_data/surface_test.cpp#L78-L89
Fixed unit test |
taylorbrown75
left a comment
taylorbrown75
left a comment
There was a problem hiding this comment.
Updated unit test results and reverted time.
|
Good work and nice catch! |
Legacy soltrace uses cx and cy for parabola arguments. The new structure uses focal lengths x and y.
Modified conversion step. Modified test time for NativeRunner LargePerformanceTest.