Restructured backend for SolTrace #63

jmaack24 · 2025-04-17T15:52:38Z

Work in progress:

Simulation data structure implementation
Simulation runner implementation for existing CPU SolTrace backend
Validation and regression tests for native runner
Simulation result implementation for native runner

…mplementation

…ses; start implementing APIs

… CMake to use cpp 17.

… tower test

…n on the powertower sample

…o see how it runs

…e name

…to build

…n result storage; move ray data to shared pointer

Copilot

Pull Request Overview

Copilot reviewed 124 out of 147 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-01T15:55:39Z

google-tests/test_tools/split_csv.cpp

+#include <fstream>
+#include <iostream>
+#include <string>
+#include <sstream>
+#include <vector>
+
+using namespace std;


Avoid using 'using namespace std;' in header files or at global scope. It pollutes the global namespace and can cause naming conflicts. Use specific using declarations or fully qualified names instead.

The using statement is in a .cpp file. Probably not something that needs to be addressed.

Copilot · 2025-10-01T15:55:39Z

google-tests/soltrace_example_tests/mtrand_test.cpp

 	MTRand myrng(1);
 	double random_number = myrng.rand();

-	EXPECT_NEAR(random_number, 0.13387664401253274, 0.0000001);
+	EXPECT_NEAR(random_number, 0.13387664401253274, 1e-7);


Consider using a named constant for the expected value and tolerance to improve readability and maintainability. Magic numbers make the test harder to understand and modify.

This is a test for the legacy code and will likely be deprecated at some point.

Copilot · 2025-10-01T15:55:40Z

coretrace/simulation_runner/native_runner/tracing_errors.cpp

+			goto Label_9;
+		}
+		else
+		{
+			Euler[0] = PI / 2.0;
+			goto Label_8;
+		}
+	}
+
+	Euler[0] = atan2(CosIn[0], CosIn[2]);
+Label_8:
+	Euler[1] = atan2(CosIn[1], sqrt(CosIn[0] * CosIn[0] + CosIn[2] * CosIn[2]));
+Label_9:


Use of goto statements makes code harder to read and maintain. Consider refactoring this logic using structured control flow (if/else, functions, or early returns) instead.

Suggested change

goto Label_9;

}

else

{

Euler[0] = PI / 2.0;

goto Label_8;

}

}

Euler[0] = atan2(CosIn[0], CosIn[2]);

Label_8:

Euler[1] = atan2(CosIn[1], sqrt(CosIn[0] * CosIn[0] + CosIn[2] * CosIn[2]));

Label_9:

}

else

{

Euler[0] = PI / 2.0;

Euler[1] = atan2(CosIn[1], sqrt(CosIn[0] * CosIn[0] + CosIn[2] * CosIn[2]));

}

}

else

{

Euler[0] = atan2(CosIn[0], CosIn[2]);

Euler[1] = atan2(CosIn[1], sqrt(CosIn[0] * CosIn[0] + CosIn[2] * CosIn[2]));

}

Noted in #71 and to be addressed in a future PR.

Copilot · 2025-10-01T15:55:40Z

coretrace/simulation_runner/native_runner/process_interaction.cpp

+        NIter = 10;
+        Epsilon = 0.000005;


These appear to be magic numbers that should be defined as named constants at the top of the file or as class members to improve maintainability and make their purpose clear.

Convert parabola args to from cx to fx.

taylorbrown75

Looks good. Legacy SolTrace GUI and coretrace can be built by setting CMakelist variables to ON. Legacy sample files run, tests pass on my machine.

qualand · 2025-10-15T16:58:04Z

.github/workflows/CI.yml

 on:
  push:
-    branches: [ develop, trace_refactor ]
+    branches: [ develop, trace_refactor, jm-dev-tmb ]


remove trace_refactor on both push and PRs

Returned to just develop

Fix focal length check bug.

qualand · 2025-10-15T18:44:58Z

coretrace/simulation_data/cst_templates/heliostat.cpp

+    return;
+}
+
+void Heliostat::enforce_user_fields_set() const


We are missing some of the class user set fields here, e.g., gap_x.

I see your checking them as they are set.

It should still get checked by the enforce_user_fields_set function. I will add it.

qualand · 2025-10-15T18:46:36Z

coretrace/simulation_data/cst_templates/heliostat.cpp

+{
+    if (this->initialized)
+    {
+        CompositeElement::enforce_user_fields_set();


As written, I believe this will always be skipped. The template only calls this function if initialized is False.

The enforce_user_fields_set function gets called when the element is added to a SimulationData object as well, at which point it is initialized and this will get called.

qualand · 2025-10-15T18:55:23Z

coretrace/simulation_data/cst_templates/heliostat.cpp

+            elem->set_aperture(ap);
+
+            // // TODO: Is this the correct thing to do here?
+            // vector_add(1.0, this->focal_point, -1.0, origin, aim);


What is the focal_point? Should this and the other code referring to it be removed?

Probably something from when I was trying to implement it. I will remove it.

qualand · 2025-10-15T18:55:43Z

coretrace/simulation_data/cst_templates/heliostat.cpp

+            if (this->focal_length_x <= 0.0 && this->focal_length_y <= 0.0)
+            {
+                surf = make_surface<Flat>();
+                // Vector3d khat(0.0, 0.0, 1.0);


qualand · 2025-10-15T18:58:54Z

coretrace/simulation_data/cst_templates/heliostat.cpp

+                                               0.0);
+
+            elem->set_front_optical_properties(this->optics_mirror);
+            elem->set_back_optical_properties(this->optics_mirror);


We should probably make the back surface properties different. Either fixed to some default "absorber" or provide access to customize.

I will add a TODO about this.

It was while doing these templates that I realized we should rework the optical properties to have be a single object with a front and back so we can do error checking make it a bit easier to create elements (less verbose).

qualand

Great start. I think we have a lot of loose ends to build out in the coming months.

qualand · 2025-10-15T19:05:59Z

coretrace/simulation_data/cst_templates/heliostat.cpp

+
+    sun_position_vector_degrees(this->sun_position, azimuth, elevation);
+    Vector3d target_dir;
+    vector_add(1.0, this->target_pos,


Should we add a check target_set before these calculations?

This gets checked by the enforce_user_fields_set function which is called at the top of the initialize function.

qualand · 2025-10-15T19:12:45Z

coretrace/simulation_data/cst_templates/heliostat.cpp

+        throw std::invalid_argument(ss.str());
+    }
+
+    this->coordinates_initialized = false;


Should we add a check on this->initialized? that either calls create_geometry or throws an error.

Yes. Added a check and an error throw.

qualand · 2025-10-15T19:33:37Z

coretrace/simulation_data/cst_templates/heliostat.cpp

+    return;
+}
+
+void Heliostat::enforce_user_fields_set() const


I see your checking them as they are set.

qualand · 2025-10-15T19:59:16Z

coretrace/simulation_data/optical_properties.hpp

+    REFRACTION
+};
+
+// enum DistributionType


qualand · 2025-10-15T20:19:03Z

coretrace/simulation_data/single_element.cpp

+                                 optics_front(),
+                                 optics_back()
+{
+    // TODO: Do we want a default aperture and surface? What about default


I would set the default to a flat rectangle (1x1) set to a perfect absorber.

Aperture and surface have checks to make sure they get set. I updated the optics to default to ideal absorber.

qualand · 2025-10-15T20:23:16Z

coretrace/simulation_data/stage_element.cpp

+
+namespace SolTrace::Data {
+
+StageElement::StageElement(int_fast64_t stage) : CompositeElement()


Is this how you envision Stages being handled? Will this implementation run into issues when the added element is a compositeElement, e.g., a CST template.

Adding a composite element to a stage is allowed so you can add a CST template to a stage. But this should work to handle stages since they are largely a way for users to define a local coordinate system.

jmaack24 and others added 25 commits February 27, 2025 15:21

Initial API for refactored SolTrace

d883262

Updates to various APIs; first pass at element/ray source container i…

77db261

…mplementation

Use existing matrix-vector code as backend for matrix and vector clas…

049e4bf

…ses; start implementing APIs

Created new test on power tower example file with 50000 rays. Changed…

29d9ba4

… CMake to use cpp 17.

Added ground results csv to new folder in google-tests directory

0afdb21

Removed register prefix from variables in mtrand.h. More errors to come.

8eecaa9

Removed lines in CMakeLists causing linux failure, fixed bug in power…

724296b

… tower test

Created parabola.stinput test and a test using powertower optimizatio…

e22de2c

…n on the powertower sample

Commented out parabola test for now, added nonexistent branch to CI t…

c5094de

…o see how it runs

Fixed syntax error

345f1ff

Changed tests based on PR feedback

f524a82

Fixed error in CMakeLists that caused failure on windows

81cf087

Removed output messages from st_sim_run_test

ba5e1d6

Fixed silly mistake in previous commit

6a9760b

Added comments and removed unused libraries

5d19b8e

Changed naming conventions of tests

87d6202

Added high flux solar furnace test changes, changed parabola test fil…

e12de7d

…e name

Fixed typo

87bab54

Merge branch 'develop' into jm-dev-branch

04731c3

Move refactored data to its own directory; add to refactored classes …

4f5d3a9

…to build

Add missed files

78bddfc

Add unit tests for basic linear algebra and container template

152c2ae

More tests; Start implementation of composite element

1f861ca

Add more unit tests on element

1639599

Add missed headers

512e13b

jmaack24 self-assigned this Apr 17, 2025

jmaack24 marked this pull request as draft April 17, 2025 15:52

jmaack24 added 3 commits April 17, 2025 10:04

Remove target from cmake build command in CI

ac29445

Initial virtual element implementation; smoke test for virtual element

698fe05

Correct aperture spelling; add tests for virtual elements

5f6a3d8

jmaack24 added 5 commits September 25, 2025 13:55

Move simulation runner and simulation result to their own namespace

18f12dc

Optix build fixes

ce378ef

Native runner report results initial implementation; update simulatio…

4b4c3de

…n result storage; move ray data to shared pointer

Rework native runner validation test; squash ray data saving bugs

08d9458

Fix linux and windows build

de4256a

jmaack24 marked this pull request as ready for review October 1, 2025 15:52

jmaack24 requested review from Copilot, qualand and taylorbrown75 October 1, 2025 15:53

Copilot AI reviewed Oct 1, 2025

View reviewed changes

taylorbrown75 and others added 7 commits October 1, 2025 10:19

Convert parabola args to from cx to fx.

d992b1d

Update test results for parabola argument change.

04c4280

Revert LargePerformanceTest time result.

18b14e0

Merge pull request #70 from NREL/parabola_args_bug

bdfad1e

Convert parabola args to from cx to fx.

Increase performance test time limit again

e4374c0

Further increase time limit

4385650

Increase other test time limit because github windows runner is slow

4ea1ac7

taylorbrown75 mentioned this pull request Oct 7, 2025

Use of goto statements makes code harder to read and maintain. Consider refactoring this logic using structured control flow (if/else, functions, or early returns) instead. #71

Open

taylorbrown75 approved these changes Oct 7, 2025

View reviewed changes

Fix focal length check bug.

b407282

qualand reviewed Oct 15, 2025

View reviewed changes

Merge pull request #72 from NREL/parabola_focal_bug

ad5b190

Fix focal length check bug.

qualand reviewed Oct 15, 2025

View reviewed changes

qualand approved these changes Oct 15, 2025

View reviewed changes

Address review comments

a4043e4

qualand merged commit 814815a into develop Oct 15, 2025
11 checks passed


		namespace SolTrace::Data {

		StageElement::StageElement(int_fast64_t stage) : CompositeElement()

Restructured backend for SolTrace #63

Restructured backend for SolTrace #63

Uh oh!

Conversation

jmaack24 commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

taylorbrown75 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qualand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jmaack24 commented Apr 17, 2025 •

edited

Loading