-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sofast examples modifications #166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review.
This script performs the following steps: | ||
1. Load saved facet ensemble SOFAST collection data from an HDF5 file. | ||
2. Save projected sinusoidal fringe images to PNG format. | ||
3. Save captured sinusoidal fringe images and mask images to PNG format. | ||
4. Process data with SOFAST and save processed data to HDF5. | ||
5. Generate a suite of plots and save image files. | ||
|
||
Examples | ||
-------- | ||
To run the script, simply execute it as a standalone program: | ||
|
||
>>> python example_process_facet_ensemble.py | ||
|
||
This will perform the processing steps and save the results to the data/output/single_facet directory | ||
with the following subfolders: | ||
1_images_fringes_projected - The patterns sent to the display during the SOFAST measurement of the optic. | ||
2_images_captured - The captured images of the displayed patterns as seen by the SOFAST camera | ||
3_processed_data - The processed data from SOFAST. | ||
4_processed_output_figures - The output figure suite from a SOFAST characterization. | ||
|
||
Notes | ||
----- | ||
- The script assumes that the input data files are located in the specified directories. | ||
- Chat GPT 40 assisted with the generation of some docstrings in this file.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script performs the following steps: | |
1. Load saved facet ensemble SOFAST collection data from an HDF5 file. | |
2. Save projected sinusoidal fringe images to PNG format. | |
3. Save captured sinusoidal fringe images and mask images to PNG format. | |
4. Process data with SOFAST and save processed data to HDF5. | |
5. Generate a suite of plots and save image files. | |
Examples | |
-------- | |
To run the script, simply execute it as a standalone program: | |
>>> python example_process_facet_ensemble.py | |
This will perform the processing steps and save the results to the data/output/single_facet directory | |
with the following subfolders: | |
1_images_fringes_projected - The patterns sent to the display during the SOFAST measurement of the optic. | |
2_images_captured - The captured images of the displayed patterns as seen by the SOFAST camera | |
3_processed_data - The processed data from SOFAST. | |
4_processed_output_figures - The output figure suite from a SOFAST characterization. | |
Notes | |
----- | |
- The script assumes that the input data files are located in the specified directories. | |
- Chat GPT 40 assisted with the generation of some docstrings in this file.""" | |
This script performs the following steps: | |
1. Load saved facet ensemble SOFAST collection data from an HDF5 file. | |
2. Save projected sinusoidal fringe images to PNG format. | |
3. Save captured sinusoidal fringe images and mask images to PNG format. | |
4. Process data with SOFAST and save processed data to HDF5. | |
5. Generate a suite of plots and save image files. | |
Examples | |
-------- | |
To run the script, simply execute it as a standalone program: | |
>>> python example_process_facet_ensemble.py | |
This will perform the processing steps and save the results to the data/output/single_facet directory | |
with the following subfolders: | |
1_images_fringes_projected - The patterns sent to the display during the SOFAST measurement of the optic. | |
2_images_captured - The captured images of the displayed patterns as seen by the SOFAST camera | |
3_processed_data - The processed data from SOFAST. | |
4_processed_output_figures - The output figure suite from a SOFAST characterization. | |
Notes | |
----- | |
- The script assumes that the input data files are located in the specified directories. | |
- Chat GPT 40 assisted with the generation of some docstrings in this file.""" | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just a few comments. Please address them and make changes as necessary.
The function for setting the positions of the facets relative to one another. | ||
|
||
"""Sets the positions of the facets relative to the ensemble. | ||
NOTE: Will remove previously set facet canting rotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if set_facet_positions() removes the canting, and set_facet_canting() removes the position, then how does both the position and canting get set at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is done by set_facet_transform_list
Not very intuitive. Maybe the focus of a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a reasonable answer. Added a new issue #188
@@ -219,6 +205,9 @@ def set_facet_positions(self, positions: Pxyz): | |||
facet._self_to_parent_transform = TransformXYZ.from_V(pos) | |||
|
|||
def set_facet_canting(self, canting_rotations: list[Rotation]): | |||
"""Sets facet canting relative to ensemble. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this apply to a single facet or multiple facets? In other words, should this be changed to "set_facet_cantings"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
|
||
class OpticOrientationAbstract(ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the ABC (Abstract Base Class) inheritance? I'd have thought we would want that inheritance for any abstract class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in another PR it looks like
@@ -104,14 +101,13 @@ def add_child(self, new_child: 'OpticOrientationAbstract', new_child_to_self_tra | |||
|
|||
@property | |||
def parent(self) -> 'OpticOrientationAbstract': | |||
"""...""" | |||
"""The parent of the current Optic""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
@braden6521: Please address existing feedback and resolve conflicts |
d69b406
to
216ec49
Compare
@e10harvey, @bbean23: Thanks for the reviews both. Changes have been made. |
216ec49
to
350131e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all the feedback. Approved!
NOTE:
Depends on #163