Merged
Conversation
…ven when they are not integer format.
…rOutput, example_MirrorOutput, also solar field. Fixed broken pytests, reduced circular imports. SpotAnalysis notebooks might be broken.
…n visualization. Also all prior debug plots output.
…le_facet.py. Added comments to LoopXY.py regarding convexity check.
jehsharp
requested changes
Feb 16, 2026
Collaborator
jehsharp
left a comment
There was a problem hiding this comment.
Hey Randy, in file common/lib/csp/MirrorAbstract.py, on line 842, this string:
If "project_..." is true
is causing the Sphinx doc check to fail, it does not like that syntax. Locally I changed it to something like this:
If any project slice variable is true (ie, project_origin_slice_y)
And that syntax works. Would you mind making that small change and pushing to the PR?
Thanks so much!
…neration problem.
Collaborator
Author
|
Hi Jeff,
Done!
Please let me know if you need anything else.
Thanks,
…-Randy
From: Jeff Sharpe ***@***.***>
Sent: Monday, February 16, 2026 1:32 PM
To: sandialabs/OpenCSP ***@***.***>
Cc: Brost, Randy ***@***.***>; Author ***@***.***>
Subject: [EXTERNAL] Re: [sandialabs/OpenCSP] Sofast diagnostics (PR #328)
@jehsharp requested changes on this pull request.
Hey Randy, in file common/lib/csp/MirrorAbstract.py, on line 842, this string:
If "project_..." is true
is causing the Sphinx doc check to fail, it does not like that syntax. Locally I changed it to something like this:
If any project slice variable is true (ie, project_origin_slice_y)
And that syntax works. Would you mind making that small change and pushing to the PR?
Thanks so much!
-
Reply to this email directly, view it on GitHub<#328 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BCESO332B65UUR6DVNQRMC34MISKRAVCNFSM6AAAAACVG6FWG6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTQMJQGQZDEMRXGA>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
jehsharp
approved these changes
Feb 16, 2026
Collaborator
jehsharp
left a comment
There was a problem hiding this comment.
This looks good to me!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added Rendering of Mirror Embedded Surface, Also Bug Fixes
There has been a longstanding need to depict the relationship between a mirror's defining boundary on the (x, y) plane and its corresponding bound on the 3-d embedding optical surface. There also has been a longstanding need to draw slices of both parametric surfaces and measured point mirrors (the latter beyond the scope of this pull request). These new capabilities are also currently needed for a current challenging measurement problem. There also was a need to update and repair key test routines exercising mirror output.
Summary of changes
I extended the mirror rendering code to show the embedded surface of parametric mirrors, and also to show the relationships between defining features on the (x, y) plane and their corresponding features lifted onto the mirror embedding surface. This results in "projected" and "lifted" instances of the geometry.
This also includes implementation of general constant-x and constant-y slices of parametric mirrors, including both their projected and lifted geometries, and connecting lines between them.
These are demonstrated and tested in the test_MirrorOutput.py and example_MirrorOutput.py scripts. Further, the example_MirrorOutput.py scripts now verifies its output against expected output, as it should always have done.
I also extended the example_process_single_facet.py script to utilize this capability, to facilitate study of a new challenging measurement example.
I further began to extend the example_process_single_facet.py script to generate diagnostic output showing the progress of its computation to facilitate understanding and debugging. This work is ongoing, but I wanted to release these improvements instead of waiting until the full SOFAST diagnostic quite is operational.
I also improved some documentation and fixed some bugs encountered, most notably some circular imports which were blocking computation.
Implementation notes
It is possible that my repair to the circular references may have introduced a problem with the SpotAnalysis jupyter notebooks; I did not test these. However, the reason I conjecture that this might have occurred is due to a very large and problematic import dependence which caused the observed circular import problem. If problems with the SpotAnalysis jupyter notebooks are observed, then the SpotAnalysis code should be refactored to prevent the introduction of the multiple circular imports. (One specific example I encountered was that when a RegionXY was imported, it correctly imported a LoopXY class which in turn imported the View3d class. This is appropriate, since a RegionXY contains LoopXY objects, and both classes must be able to draw themselves. However, the problem occurred when the import of the View3d class forced an import of the entire rendering package, and then the entire computer vision package, and then large chunks of the SpotAnaylsis system. One of the SpotAnalysis files ultimately needed a RegionXY object, and this caused the circular import loop. This expansive growth of imports is excessive and should be prevented by refactoring.
Submission checklist
develop, notmainopencsp/test/test_DocStringsExist.pyare verified to include this change or have been updated accordinglydoc/are verified to include this change or have been updated accordinglyAdditional information
Work on SOFAST diagnostics will continue.