Sofast Fixed multi facet capability #164

braden6521 · 2024-08-22T22:57:22Z

Added a multi-facet measurement capability to Sofast Fixed
Created an abstract processing class for ProcessSofastFixed and ProcessSofastFringe

NOTE:

Depends on #163.

e10harvey

Thank you, @braden6521! My main concerns are the absence of bounds checks on facet indices and cleaning some of the constant values up with self documenting variables / enums.

e10harvey · 2024-10-02T21:55:40Z

contrib/test_data_generation/sofast_fringe/generate_test_data_single_facet.py

@@ -74,7 +74,7 @@ def generate_dataset(

    # Show slope map
    mask = sofast.data_image_processing_facet[0].mask_processed
-    slopes_xy = sofast.data_characterization_facet[0].slopes_facet_xy
+    slopes_xy = sofast.data_calculation_facet[0].slopes_facet_xy


nit: make 0 a Final (https://docs.python.org/3/library/typing.html#typing.Final) local variable.

Never heard of Final before. That's cool, thanks for the suggestion.

e10harvey · 2024-10-02T21:55:59Z

contrib/test_data_generation/sofast_fringe/generate_test_data_single_facet.py

@@ -74,7 +74,7 @@ def generate_dataset(

    # Show slope map
    mask = sofast.data_image_processing_facet[0].mask_processed
-    slopes_xy = sofast.data_characterization_facet[0].slopes_facet_xy
+    slopes_xy = sofast.data_calculation_facet[0].slopes_facet_xy
    slopes = np.sqrt(np.sum(slopes_xy**2, 0))


nit: same as above for 2

This looks like a duplicate of above?

e10harvey · 2024-10-02T21:57:17Z

example/sofast_fringe/example_process_facet_ensemble.py

-    # ensemble.plot_orthorectified_slope(res=0.002, clim=7, axis=fig_record.axis)
-    # fig_record.save(dir_save, 'slope_magnitude', 'png')
+    fig_record = fm.setup_figure(figure_control, axis_control_m, title='')
+    ensemble.plot_orthorectified_slope(res=0.002, clim=7, axis=fig_record.axis)


unless self-explanatory for a SME please add some docs or self documenting variables for the "0.002" and "7" magic values

Good comment. Documented.

e10harvey · 2024-10-02T21:57:33Z

example/sofast_fringe/example_process_facet_ensemble.py

@@ -145,7 +145,7 @@ def example_process_facet_ensemble():
    # 3. Log best-fit parabolic focal lengths
    # =======================================
    for idx in range(sofast.num_facets):
-        surf_coefs = sofast.data_characterization_facet[idx].surf_coefs_facet
+        surf_coefs = sofast.data_calculation_facet[idx].surf_coefs_facet
        focal_lengths_xy = [1 / 4 / surf_coefs[2], 1 / 4 / surf_coefs[5]]


nit: more magic numbers 1,4,2,5

e10harvey · 2024-10-02T21:59:31Z

opencsp/app/sofast/lib/BlobIndex.py

@@ -47,6 +48,8 @@ def __init__(self, points: Vxy, x_min: int, x_max: int, y_min: int, y_max: int)
        self._neighbor_dists = np.zeros((self._num_pts, 4)) * np.nan  # left, right, up, down

        self.search_thresh = 5.0  # pixels
+        self.max_num_iters: int = 100


Consider making this a "Final" (https://docs.python.org/3/library/typing.html#typing.Final)

Thanks for the comment. These values will likely be updated by the user, so I didn't make them Final. However, they were in desperate need of documentation. I added documentation inline and in the module docstring.

Ah, if these are defaults will the default values be obvious to the user? If not, please add the default to the doc string such as: "(default: 100)".

e10harvey · 2024-10-02T22:49:56Z

opencsp/common/lib/csp/Facet.py

@@ -206,69 +205,3 @@ def draw(self, view: View3d, facet_style: RenderControlFacet = None, transform:

        if facet_style.draw_mirror_curvature:
            self.mirror.draw(view, facet_style.mirror_styles, transform * self.mirror._self_to_parent_transform)
-


Thank you for removing these unverified methods

e10harvey · 2024-10-02T22:50:24Z

opencsp/common/lib/csp/FacetEnsemble.py

            # Get all mirror region vertices
            points_xy = Pxy.merge([loop.vertices for loop in facet.mirror.region.loops])  # mirror frame
            points_z = facet.mirror.surface_displacement_at(points_xy)  # mirror frame
+            # If the corners aren't in range of the mirror's interpolation function, set to 0
+            if np.any(np.isnan(points_z)):
+                lt.warn(f'Could not find corner z values for facet number {facet_idx:d}; filling with zeros.')


Suggested change

lt.warn(f'Could not find corner z values for facet number {facet_idx:d}; filling with zeros.')

lt.warn(f'Could not find corner z values for facet number {facet_idx:03d}; filling with zeros.')

Change made.

e10harvey · 2024-10-02T22:51:46Z

opencsp/common/lib/csp/OpticOrientationAbstract.py

-        while searcher._self_to_parent_transform is not None:
-            if searcher is target:
-                return transform
+        while searcher.parent is not None:


consider filing a follow-on issue if these linear searches are expected to be a bottleneck

Sounds good. As we use this capability more, I will keep this in mind.

e10harvey · 2024-10-02T22:53:14Z

opencsp/test/data/sofast_common/Ensemble_split_NSTTF_facet.json

@@ -0,0 +1,18 @@
+{


consider filing a follow-on issue to randomly generate this test data at runtime.

I still do not fully understand how to use random numbers in tests. Any suggestions?

Let's discuss this. Even if it is a more robust test, it may not be straight forward or practical to generate random values.

e10harvey · 2024-10-02T22:53:20Z

opencsp/test/data/sofast_common/Facet_split_NSTTF.json

@@ -0,0 +1,10 @@
+{


Same comment as above. These specific JSON files define the physical shape of the facet being measured. But if there is more opportunity to use randomness in these tests, I could implement it.

e10harvey · 2024-10-09T15:40:26Z

example/sofast_fringe/example_process_facet_ensemble.py

-    # ensemble.plot_orthorectified_slope(res=0.002, clim=7, axis=fig_record.axis)
-    # fig_record.save(dir_save, 'slope_magnitude', 'png')
+    res = 0.002  # meter, make the plot with 2mm spatial resolution
+    clim = 7  # mrad, draw the plot with +/-7mrad scale bars, this mirror has erorrs that extent to about +/-7mrad


Suggested change

clim = 7 # mrad, draw the plot with +/-7mrad scale bars, this mirror has erorrs that extent to about +/-7mrad

clim = 7 # mrad, draw the plot with +/-7mrad scale bars, this mirror has erorrs that extend to about +/-7mrad

Good catch 🤦‍♂️

e10harvey · 2024-10-09T15:43:10Z

opencsp/app/sofast/lib/BlobIndex.py

@@ -47,6 +48,8 @@ def __init__(self, points: Vxy, x_min: int, x_max: int, y_min: int, y_max: int)
        self._neighbor_dists = np.zeros((self._num_pts, 4)) * np.nan  # left, right, up, down

        self.search_thresh = 5.0  # pixels
+        self.max_num_iters: int = 100


Ah, if these are defaults will the default values be obvious to the user? If not, please add the default to the doc string such as: "(default: 100)".

e10harvey · 2024-10-09T15:53:40Z

opencsp/app/sofast/lib/ProcessSofastAbstract.py

+
+        # Facet definition
+        if self.data_facet_def is not None:
+            for idx_facet, facet_data in enumerate(self.data_facet_def):


If you anticipate any bugs related to values larger than 999, I would strongly suggest adding a bounds check where appropriate to ensure that that largest facet index does not exceed 999, and, if it does, throw an exception stating that this is not permitted. I confirmed that the width specifier does not truncate values larger than 999 in the f-strings, so that should be fine.

e10harvey · 2024-10-09T15:57:18Z

opencsp/test/data/sofast_common/Ensemble_split_NSTTF_facet.json

@@ -0,0 +1,18 @@
+{


Let's discuss this. Even if it is a more robust test, it may not be straight forward or practical to generate random values.

… perimeter xyz points

…r single and multi facets.

… be found.

…cess sofast class.

…facet

…uration class.

…e.py

braden6521 self-assigned this Aug 29, 2024

braden6521 added the enhancement New feature or request label Aug 29, 2024

braden6521 requested review from e10harvey and bbean23 August 29, 2024 21:03

braden6521 marked this pull request as ready for review August 29, 2024 23:14

e10harvey requested changes Oct 2, 2024

View reviewed changes

braden6521 force-pushed the sofast_fixed_multi branch from b71822b to 36e9691 Compare October 8, 2024 22:48

e10harvey reviewed Oct 9, 2024

View reviewed changes

braden6521 added 22 commits October 9, 2024 15:41

Reorganized ProcessSofastFixed function names.

b4fd325

Added optic_type and num_facets to process_single_facet_optic

8d1be2e

Surface and facet data are now lists.

bc2cb49

Data surface and facet data are lists.

f22b8fc

Updated documentation in PSF

32e4a79

fixed docs in process_optics_geometry

5ff9374

Updated test sofast fixed (for single facet only)

64e532d

Fixed documentation in PSFr

b924dd7

Made slope_solver and data lists in PSFi

b5e8bcb

First implimentation of PSFi multi

0ca1398

Updated documentation and typing in PSFi.

4409986

Fixed error in process_optics_geometry when finding expected ensemble…

1d2ebcf

… perimeter xyz points

Updated mask histogram calculations in image_processing.

9bdbfe9

Fixed error with variable name in calc_mask_raw

c688cce

Changed surface to surfaces

1d9fb9c

ProcessSofastFixed now takes in origin point and xy index as input fo…

24aaf37

…r single and multi facets.

Added docs, stub, and checks to blobIndex

f66761f

Added get-data_in_region to BlobIndex

46cbf3d

Added facet pointing and ensemble output to processSofastFixed

0e911d5

Added check in FacetEnsemble that catches when corner z values cannot…

f8ad6f5

… be found.

Fixed testProcessSofastFixed

e5a45af

Made slope_solver attribute a list in ProcessSofastFixed

7883af0

braden6521 added 26 commits October 9, 2024 15:41

Fixed _000 prefix in SofastFixed HDF5 save output

b37a01f

Updated testProcessSofastFixed to include ensemble testing.

3553e1b

Unified PSFr and PSFi data container names

2a0d0ee

Changed get_mirror to get_optic in SFi

90e6b48

Changed slope_solver to slope_solvers

0924ae7

Instantiated ProcessSofastAbstract and integrated into SFr and SFi

00a1748

Added save_to_hdf to PSA.

f1a5257

Added facet pointing test data to SFi calcultion HDF5

b0cbd4a

Added facet pointing test to SFi test

ac67069

Moved get_optic and _calculate_facet_pointing methods to abstract pro…

d3d63c1

…cess sofast class.

Updated all calls to data_characterization_facet to data_calculation_…

61a4e42

…facet

Revert of removing ABC from OO

6fc92cd

Fixed reference to slope_solver and slope_solver_data in SofastConfig…

6dd5d97

…uration class.

Fixed typing in generate_dataset()

71ba3a6

Got rid or explianed 'magic numbers' in example process facet ensembl…

6c226ac

…e.py

Added typing argument in ProcessSofastFringe

43c6422

Used Enum in BlobIndex step.

99d7d0d

Fixed typo in Processsofastabstract

0d6be36

Updated readability in ProcessSofastAbstract

c6a83b8

Updated typing in PSF

4f38436

Updated documentation in test PSF

0c35060

updated docs in test_sofastConfiguration

d798e1c

updated docs in FacetEnsemble

0591705

Updated calculated facet hdf5

14ab577

Fixed typo

3d7640c

Added defaults in docstrings

18e7b4e

braden6521 force-pushed the sofast_fixed_multi branch from 83cfa3a to 18e7b4e Compare October 9, 2024 21:44

e10harvey approved these changes Oct 9, 2024

View reviewed changes

e10harvey merged commit 3fda296 into sandialabs:develop Oct 9, 2024
4 checks passed

braden6521 deleted the sofast_fixed_multi branch December 6, 2024 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sofast Fixed multi facet capability #164

Sofast Fixed multi facet capability #164

braden6521 commented Aug 22, 2024 •

edited

Loading

e10harvey left a comment

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 9, 2024

e10harvey Oct 2, 2024

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 9, 2024

e10harvey Oct 2, 2024

braden6521 Oct 8, 2024

e10harvey Oct 9, 2024

braden6521 Oct 9, 2024

e10harvey Oct 9, 2024

e10harvey Oct 9, 2024

e10harvey Oct 9, 2024

		@@ -206,69 +205,3 @@ def draw(self, view: View3d, facet_style: RenderControlFacet = None, transform:

		if facet_style.draw_mirror_curvature:
		self.mirror.draw(view, facet_style.mirror_styles, transform * self.mirror._self_to_parent_transform)

	lt.warn(f'Could not find corner z values for facet number {facet_idx:d}; filling with zeros.')
	lt.warn(f'Could not find corner z values for facet number {facet_idx:03d}; filling with zeros.')

	clim = 7 # mrad, draw the plot with +/-7mrad scale bars, this mirror has erorrs that extent to about +/-7mrad
	clim = 7 # mrad, draw the plot with +/-7mrad scale bars, this mirror has erorrs that extend to about +/-7mrad

Sofast Fixed multi facet capability #164

Sofast Fixed multi facet capability #164

Conversation

braden6521 commented Aug 22, 2024 • edited Loading

NOTE:

e10harvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braden6521 commented Aug 22, 2024 •

edited

Loading