-
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
SofastFixed unit tests and test data #63
SofastFixed unit tests and test data #63
Conversation
…era_sofast_downsampl. Updated file paths in unit tests.
710faa1
to
fbcb5b9
Compare
fbcb5b9
to
d1ec52c
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.
I have a few comments, but you can feel free to take them or leave them.
@@ -41,19 +36,13 @@ def generate_data(): | |||
im_ds = ddg.downsample_images(im, n_downsample) | |||
# Save | |||
file_save = join(dir_save, 'images', basename(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.
FYI, the "opencsp" way to get the basename is with file_tools.path_components():
file_path, file_name, file_ext = ft.path_components(file)
file_save = join(dir_save, 'images', file_name+file_ext)
This is a little more verbose, and sometimes it is nicer to just use basename. Up to you.
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 the tip! I like that; in this particular instance, I'll probably keep it as-is because it's simpler, but I'll keep this in mind!
@@ -41,19 +36,13 @@ def generate_data(): | |||
im_ds = ddg.downsample_images(im, n_downsample) | |||
# Save | |||
file_save = join(dir_save, 'images', basename(file)) | |||
imageio.imwrite(file_save, im_ds, quality=70) |
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.
Ah yes, the old struggle. Better images or smaller files?
FYI: because .png and .gif images use a pallet of colors, you can sometimes get smaller files with lossless quality by using one of these formats.
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 the tip! I think these images might be too high of resolution. I tried png and gif and couldn't get the images smaller than JPG. I'll keep this in mind for the future though!
lt.info(f'Saving figure to: {file:s}') | ||
fig.savefig(file) | ||
|
||
|
||
def example_driver(): |
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.
Did you mean to delete this method? If yes, then great! I just wanted to make sure you're aware of it.
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.
Yes, since the example_driver()
now only directly called example_calibation_screen_shape()
I just called that directly.
2. Processes data with SOFAST | ||
3. Prints best-fit parabolic focal lengths | ||
4. Plots slope magnitude, physical setup | ||
1. Loads saved single facet Sofast collection data |
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.
For particularly long or complicated functions, I also like to describe the steps just like this.
My own personal style is to take this a step further; I add comments in the code with the step numbers. For example:
# 1. Load data
...
# 2. Process
...
# 3. Calculate and log focal length from parabolic fit
...
# 4. Plot slope map
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.
Yes, I like it! That would make the examples much easier to follow! Inline numbers are now added.
Directory to save standard output image files | ||
focal_length_paraboloid : float | ||
Focal length of ideal symmetric parabolid | ||
1) Creates an OpenCSP representation of facet measured by Sofast |
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.
🥳
lt.logger(join(dir_save, 'log.txt'), lt.log.INFO) | ||
|
||
# Define input files | ||
file_pts_data = join(opencsp_code_dir(), 'test/data/sofast_common/aruco_corner_locations.csv') |
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.
Should the data be moved to an 'input' subdirectory 'test/data/input/sofast_common/aruco_corner_locations.csv'?
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.
Yes... I remember thinking this was sloppy and remember thinking that I should totally change it.
Measurement / expected data files are now in representative folders. All unit tests/examples updated.
@bbean23, thanks for the comments as usual. I really like the changes. Ready for review again; take a look when you get a chance. |
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.
Looks good. Approved!
Vision for Sofast Fixed/Fringe