Finished Sofast HDF5 IO #31

braden6521 · 2024-03-18T21:22:18Z

Finished the SofastFringeProcess class's HDF5 saving infrastructure.

All of ProcessSofastFringe's data classes are saved/loaded to HDF5 files automatically.
Cleaned up some random files in /test/data/output/ that weren't supposed to be in repo.
Moved an example file (view_camera_distortion) that was accidentally in the wrong folder
Created HDF5_IO_Abstract() and HDF_SaveAbstract() classes to hdf5_tools.py. These define standard input/output methods to save to HDF5 files.
Added HDF input/output methods to Surface2Dxxx classes.
Added HDF input/output unit tests to Surface2Dxxx classes.
Sofast now inherits from HDF_SaveAbstract()
Modified SofastFringe so that input surface definitions are no longer dictionaries, but rather Surface2DParabolic or Surface2DPlano classes. (The old dictionary input method was basically a dictionary version of a data class). Now, all data classes within SofastFringe now support HDF5 saving natively.
Updated SofastFringe integration tests to use Surface2Dxxx classes.
Updated the SofastFringe examples to use this new surface definition method.
Added use of logtools to SofastFringe examples (Thanks to Bens' awesome idea).
Added use of logtools to ProcessSofastFringe class.
Added use of filetools to SofastFringe examples
Renamed SofastFringe examples so names follow coarse->fine convention.

e10harvey · 2024-03-19T17:19:39Z

@braden6521: Please rebase this on-top of develop.

bbean23

Please address all comments and ping me, so I can approve then.

bbean23 · 2024-03-19T17:05:16Z

example/sofast_fringe/example_process_facet_ensemble.py

+    ft.create_directories_if_necessary(dir_save)
+
+    # Set up logger
+    lt.logger(join(dir_save, 'log.txt'))


It's typically better to do this step outside a function, such as in the if __name__ == "__main__": control, because lt.logger() should only be called once per process.

That being said, I think it's ok to leave it here, since the example_driver() function essentially is the main process.

Sounds good, I'll make sure to do that in the future.
I'll leave it here for the time being for the reason you mentioned. Also the save_dir needs to be set up before the log, so I'm fine with leaving it in this file.

bbean23 · 2024-03-19T17:07:44Z

opencsp/app/sofast/lib/process_optics_geometry.py

@@ -410,6 +410,37 @@ def process_multifacet_geometry(
    list[cdc.CalculationImageProcessingFacet],
    cdc.CalculationError,
 ]:
+    """Processes optic geometry for multifacet deflectometry measurement


Yesssssssssssssss! Love me some docstrings!

bbean23 · 2024-03-19T17:08:28Z

opencsp/app/sofast/lib/process_optics_geometry.py

+    ensemble_data : DefinitionEnsemble
+        Ensemble definition object
+    mask_raw : ndarray
+        Raw calculated mask


What is the type of the mask? Is it a boolean WxH array?

Documentation updated

bbean23 · 2024-03-19T17:17:47Z

opencsp/app/sofast/lib/process_optics_geometry.py

+    calculation_data_classes.CalculationImageProcessingGeneral
+    list[calculation_data_classes.CalculationDataGeometryFacet]
+    list[calculation_data_classes.CalculationImageProcessingFacet]
+    calculation_data_classes.CalculationError]


Two notes here:

If you'd like, you can give example names and also descriptions in your return values. I at least find it helpful. I pulled the variable names from 300 lines below here and made up some example descriptions:

""" ... Returns ------- data_geometry_general: calculation_data_classes.CalculationDataGeometryGeneral A general form describing the geometry of the facet. data_image_processing_general: calculation_data_classes.CalculationImageProcessingGeneral A general form describing what processing was done to the input images to this process_multifacet_geometry() function. data_geometry_facet: list[calculation_data_classes.CalculationDataGeometryFacet] The geometry of each facet, one per facet. Index 0 is the top left facet, and index -1 is the bottom right facet. They are listed in left-to-right, top-to-bottom order. (ie index 1 == facet in the top row, second from left, index 2 == ...) data_image_processing_facet: list[calculation_data_classes.CalculationImageProcessingFacet] The image processing that was done to each facet. data_error: calculation_data_classes.CalculationError] All the errors encountered during image processing. """

Is the last return supposed to also be a list?

Documentation updated, thanks for the suggestion.

Not supposed to be a list, just a copy-paste error (I fixed all instances of this error).

bbean23 · 2024-03-19T17:19:33Z

opencsp/app/sofast/test/test_integration_single_facet.py

-    Test.test_int_points()
-
-    print('All tests run.')
+    unittest.main()


Thank you! I highly prefer this style of calling unit tests! 🥳

Same, I've recently started converting to this way.

bbean23 · 2024-03-19T17:20:38Z

opencsp/common/lib/deflectometry/ParamsSlopeSolverParaboloid.py

+
+
+@dataclass
+class ParamsSlopeSolverParaboloid:


Should this inherit from ParamsSlopeSolver?

Wow, I just left these classes unfinished...🤦‍♂️ ParamsSlopeSolver is now ParamsSlopeSolverAbstract and is inherited by ParamsSlopeSolverParabolic and ParamsSlopeSolverPlano. Thanks for the catch!

Code reviews ftw

bbean23 · 2024-03-19T17:49:45Z

opencsp/common/lib/deflectometry/Surface2DParabolic.py

@@ -327,3 +328,35 @@ def shift_all(self, v_align_optic_step: Vxyz) -> None:
        self.v_optic_cam_optic += v_align_optic_step
        self.v_screen_points_optic += v_align_optic_step
        self.v_optic_screen_optic += v_align_optic_step
+
+    def save_to_hdf(self, file: str, prefix: str = ''):


This is more of a preference comment than a "best practices" or "requirements" comment. I like naming my conversion methods starting with either "to_" or "from_", and then they're easy to identify with the IDE. What do you think?

My current convention is "save_to_xxx" or "load_from_xxx." (I believe I have HDF5 and CSV methods with that convention). But I didn't put too much thought into those names; I'm totally fine switching to the standard you mentioned. Shall we move toa to_csv, form_csv, to_hdf, from_hdf, etc. standard.

Let's use to_* and from_*. From some quick googling those seem to be the more standard way of naming conversion methods.

Or, simply save(file_path_name_ext: str) and throw an error for anything that isn't '.h5'. Either way is good.

@bbean23, sounds good! I will likely put this in separate PR so this one doesn't get too cluttered.

bbean23 · 2024-03-19T17:54:00Z

opencsp/common/lib/tool/hdf5_tools.py

+    """Abstract class for saving to HDF5 format"""
+
+    @abstractmethod
+    def save_to_hdf(self, file: str, prefix: str = '') -> None:


I really like this interface! Having a "prefix" should really clean up how we interact with HDF5 files.

braden6521 · 2024-03-19T20:20:06Z

@braden6521: Please rebase this on-top of develop.

Done. This branch has been rebased on top of develop.

braden6521 · 2024-03-19T20:36:57Z

@bbean23, I believe I addressed all your comments.

bbean23

Looks good! Last thing to do is change save_to_hdf5->to_hdf5 and load_from_hdf5->from_hdf5. However, that can happen whenever. Feel free to merge.

…imented yet. Fixing merge conflicts.

Fixed merge conflicts in hdf5 tools

Fixed merge conflict in test_Surface2D.py

Fixed merge conflicts in multifacet example and ProcessSofastFringe

Fixed merge conflicts in undefined shape sofast example

bbean23

Still looks good after the rebase. Approved!

braden6521 self-assigned this Mar 18, 2024

braden6521 requested a review from bbean23 March 18, 2024 21:22

braden6521 marked this pull request as draft March 18, 2024 21:23

braden6521 marked this pull request as ready for review March 18, 2024 22:20

bbean23 reviewed Mar 19, 2024

View reviewed changes

braden6521 force-pushed the surface_params branch from a802ab6 to 54c8b7c Compare March 19, 2024 19:24

bbean23 previously approved these changes Mar 20, 2024

View reviewed changes

braden6521 added 16 commits March 20, 2024 13:18

Removed unused output hdf file

82e1e1d

Migrated surface_params to directly use Surface2Dxxx. Saving not impl…

0cbd455

…imented yet. Fixing merge conflicts.

Added HDF5 IO abstract classes to hdf5_tools

fa166af

Fixed merge conflicts in hdf5 tools

Added save/load methods to Surface2D... classes

2f7de79

Fixed merge conflict in test_Surface2D.py

Single facet stable

a12b184

Aded missing documentation to process_multifacet_geometry

1ffe71d

Migrated SofastFringe to use logging and Surface2Dxx classes only.

ace0620

Fixed merge conflicts in multifacet example and ProcessSofastFringe

Reorganized imports in SofastFringe examples

5b3745c

Changed names of sofastFringe examples

d71af33

Moved view camera distortion ex to correct folder

38ca87f

Updated SofastFringe integration tests

6ed65ba

Fixed merge conflicts in undefined shape sofast example

Added matplotlib import

57a239f

Updated test_SlopeSolver to use Surface2D classes

697f3fc

Added docstring

027e66e

Updated documentation in process_optics_geometry

f050465

Renamed ParamsSlopeSolver to ParamsSlopeSolverAbstract

19afc0c

braden6521 dismissed bbean23’s stale review via 19afc0c March 20, 2024 19:32

braden6521 force-pushed the surface_params branch from 651acf1 to 19afc0c Compare March 20, 2024 19:32

bbean23 approved these changes Mar 20, 2024

View reviewed changes

braden6521 merged commit 5c3a408 into sandialabs:develop Mar 20, 2024
0 of 3 checks passed

braden6521 deleted the surface_params branch April 15, 2024 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finished Sofast HDF5 IO #31

Finished Sofast HDF5 IO #31

braden6521 commented Mar 18, 2024 •

edited

Loading

e10harvey commented Mar 19, 2024

bbean23 left a comment

bbean23 Mar 19, 2024

braden6521 Mar 19, 2024

bbean23 Mar 19, 2024

bbean23 Mar 19, 2024

braden6521 Mar 19, 2024

bbean23 Mar 19, 2024

braden6521 Mar 19, 2024

bbean23 Mar 19, 2024

braden6521 Mar 19, 2024

bbean23 Mar 19, 2024

braden6521 Mar 19, 2024

bbean23 Mar 20, 2024

bbean23 Mar 19, 2024

braden6521 Mar 19, 2024

bbean23 Mar 20, 2024

bbean23 Mar 20, 2024

braden6521 Mar 20, 2024

bbean23 Mar 19, 2024

braden6521 commented Mar 19, 2024

braden6521 commented Mar 19, 2024

bbean23 left a comment

bbean23 left a comment

Finished Sofast HDF5 IO #31

Finished Sofast HDF5 IO #31

Conversation

braden6521 commented Mar 18, 2024 • edited Loading

Finished the SofastFringeProcess class's HDF5 saving infrastructure.

e10harvey commented Mar 19, 2024

bbean23 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

braden6521 commented Mar 19, 2024

braden6521 commented Mar 19, 2024

bbean23 left a comment

Choose a reason for hiding this comment

bbean23 left a comment

Choose a reason for hiding this comment

braden6521 commented Mar 18, 2024 •

edited

Loading