Skip to content

Conversation

@plexoos
Copy link
Member

@plexoos plexoos commented Aug 4, 2025

No description provided.

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 17:12

outdated suggestion

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 17:21

outdated suggestion

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 17:22

outdated suggestion

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 17:35

outdated suggestion

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 17:36

outdated suggestion

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 17:55

outdated suggestion

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 18:02

outdated suggestion

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 18:12

outdated suggestion

github-actions[bot]

This comment was marked as resolved.

@github-actions github-actions bot dismissed their stale review August 4, 2025 18:15

outdated suggestion

@plexoos plexoos force-pushed the performance-studies branch from 5c85f89 to 7b8a75d Compare August 18, 2025 23:55
@plexoos plexoos force-pushed the performance-studies branch from 5142852 to b60690d Compare September 3, 2025 14:48
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cpp-linter Review

Used clang-format v18.1.3

Click here for the full clang-format patch
diff --git a/src/g4appmt.h b/src/g4appmt.h
index f0f0020..88258fc 100644
--- a/src/g4appmt.h
+++ b/src/g4appmt.h
@@ -125,2 +125 @@ struct PhotonHit : public G4VHit
-        G4cout << "Detector id: " << fid << " energy: " << fenergy << " nm"
-               << " time: " << ftime << " ns"
+        G4cout << "Detector id: " << fid << " energy: " << fenergy << " nm" << " time: " << ftime << " ns"

Have any feedback or feature suggestions? Share it here.

@github-actions github-actions bot dismissed their stale review September 3, 2025 15:06

outdated suggestion

@plexoos plexoos force-pushed the performance-studies branch 2 times, most recently from c50baae to c9db7a7 Compare October 13, 2025 17:12
@plexoos plexoos force-pushed the performance-studies branch from c9db7a7 to fb9a812 Compare November 7, 2025 17:54
@github-actions github-actions bot dismissed their stale review December 16, 2025 16:57

outdated suggestion

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cpp-linter Review

Used clang-format v18.1.3

Click here for the full clang-format patch
diff --git a/CSG/csg_intersect_leaf_newcone.h b/CSG/csg_intersect_leaf_newcone.h
index 01a9845..fe8068e 100644
--- a/CSG/csg_intersect_leaf_newcone.h
+++ b/CSG/csg_intersect_leaf_newcone.h
@@ -101,2 +101,2 @@ void intersect_leaf_newcone( bool& valid_isect, float4& isect, const quad& q0, c
-    const float t_cand = fminf(roots) ; 
-    
+    const float t_cand = fminf(roots) ;
+
@@ -106,11 +106,3 @@ void intersect_leaf_newcone( bool& valid_isect, float4& isect, const quad& q0, c
-        float3 intersection_point = make_float3(
-            o.x + t_cand * d.x,
-            o.y + t_cand * d.y,
-            o.z + t_cand * d.z
-        );
-        float3 n = normalize(make_float3(
-            intersection_point.x,
-            intersection_point.y,
-            (z0 - intersection_point.z)*tth2
-        ));
-
+        float3 intersection_point = make_float3(o.x + t_cand * d.x, o.y + t_cand * d.y, o.z + t_cand * d.z);
+        float3 n =
+            normalize(make_float3(intersection_point.x, intersection_point.y, (z0 - intersection_point.z) * tth2));

Have any feedback or feature suggestions? Share it here.

@github-actions github-actions bot dismissed their stale review December 18, 2025 16:15

outdated suggestion

Move gun so the opticks_raindrop.gdml has photons created
@ggalgoczi
Copy link
Contributor

I confirm that when moving the particlegun to 0,0,0 mm (I just pushed this change into the branch) and running
optiphy/tools/run_performance.py

I see performance data created in timing_optix.txt and timing_geant.txt files. Namely the simulation times are respecitvely 0.8 s and 11.7 s. Let me know @plexoos if we can close this PR.

@ggalgoczi
Copy link
Contributor

@plexoos could you take a look?

@plexoos
Copy link
Member Author

plexoos commented Jan 6, 2026

Here is a basic test I use in CI to compare the G4 and OptiX outputs:
https://github.com/BNLNPPS/eic-opticks/blob/main/tests/compare_ab.py

Can we run it on the output from these performance jobs?

@ggalgoczi
Copy link
Contributor

Since we use the "Minimal" setting there are no such record files in my understanding, are there?

I found a bug in the geometry. The ordering of the surfaces was incorrect, fixed it in 4101e08

Also I added printing out number of Geant4 hits in the executable in 7fd2968

Now when I run 2 events I get matching number of hits from EIC-Opticks and Geant4:

G4WT0 > PhotonSD::EndOfEvent Number of PhotonHits: 14198
G4WT0 > PhotonSD::EndOfEvent Number of PhotonHits: 11896

Opticks: NumHits: 26076

We agreed that this the requirement to merge this PR. If you need any more details please open an other PR.

@plexoos
Copy link
Member Author

plexoos commented Jan 6, 2026

Have you verified that the hits have the same or similar coordinates?

@ggalgoczi
Copy link
Contributor

No, this PR should be for performance study, not a full validation.

@ggalgoczi
Copy link
Contributor

If you want feel free to open a "raindrop geometry validation" PR.

@plexoos
Copy link
Member Author

plexoos commented Jan 6, 2026

How do we know that these jobs produce reasonable hits?

@ggalgoczi
Copy link
Contributor

Define reasonable

@plexoos
Copy link
Member Author

plexoos commented Jan 7, 2026

Sure — here are a couple of metrics we could use, for example: The hit coordinate distributions statistically agree and are constrained by the sensitive detector

@ggalgoczi
Copy link
Contributor

They are, see attached plot.

plotpositions

@plexoos
Copy link
Member Author

plexoos commented Jan 7, 2026

Great! At least the circles seem to align. Can you point me to the output files?

@ggalgoczi
Copy link
Contributor

Here are the output files.
o.txt
g4.txt

@plexoos
Copy link
Member Author

plexoos commented Jan 7, 2026

After your recent changes I am getting strange results:

before:
performance_comparison

after:
performance_comparison

@ggalgoczi
Copy link
Contributor

You did not specify which changes.

@plexoos
Copy link
Member Author

plexoos commented Jan 7, 2026

1715f2e
7fd2968
4101e08

@ggalgoczi
Copy link
Contributor

You asked to validate the position of hits. I suggested let's do this in an other PR but you insisted.

Therfore I had to print out number of hits that takes time, done in 7fd2968 .

Also you asked me to plot hit positions. Therefore I had to change momentum of electron so that we can validate the hit positions. Done in 1715f2e

@ggalgoczi
Copy link
Contributor

Otherwise the electron would have exited. With this different energy different number of optical photons are created obviously.

@plexoos
Copy link
Member Author

plexoos commented Jan 7, 2026

Okay, I’ll revert these changes and merge.

There are many ways to run validations and present results. I didn’t expect to have to specify exactly how to do it. A histogram could work of course much better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants