160 clean up abstractspotanalysisimageprocessor #162
Conversation
bbean23
added
bug
…ageProcessorLeger
bbean23
force-pushed
the
160-clean-up-abstractspotanalysisimageprocessor
branch
from
cfd9edc to
0d1cbda
Compare
|
Randy Brost started reviewing this on Thursday October 31 at 9:27 AM. |
There was a problem hiding this comment.
Thank you for these changes, @bbean23. Most of my comments are related to the end-user documentation. However, I think given the nature of these changes, it makes sense to share before and after performance metrics. Is there a example or test you could run to share memory usage and runtime before & after these changes?
EDIT: Target branch should be develop rather than main.
.../common/lib/cv/spot_analysis/image_processor/test/test_AbstractSpotAnalysisImageProcessor.py
Outdated
Show resolved
Hide resolved
| self.example_array = np.zeros((40, 40, 3), dtype=np.uint8) | ||
| self.example_array[:20, :20, 0] = 255 | ||
| self.example_array[20:, :20, 1] = 255 | ||
| self.example_array[:20, 20:, 2] = 255 | ||
| self.example_array[20:, 20:, :2] = 255 |
There was a problem hiding this comment.
Please add a function that generates a randomly sized example array and sets the members you want to 255.
There was a problem hiding this comment.
Generally I agree that random input data is a good idea in unit tests. However here I disagree. I don't think that a randomly sized example array will help us to catch bugs in these unit tests and isn't worth the effort here.
Co-authored-by: Evan Harvey <[email protected]>
Co-authored-by: Evan Harvey <[email protected]>
Co-authored-by: Evan Harvey <[email protected]>
… usage calculation
| self.example_array = np.zeros((40, 40, 3), dtype=np.uint8) | ||
| self.example_array[:20, :20, 0] = 255 | ||
| self.example_array[20:, :20, 1] = 255 | ||
| self.example_array[:20, 20:, 2] = 255 | ||
| self.example_array[20:, 20:, :2] = 255 |
There was a problem hiding this comment.
Generally I agree that random input data is a good idea in unit tests. However here I disagree. I don't think that a randomly sized example array will help us to catch bugs in these unit tests and isn't worth the effort here.
| """Check memory usage and convert images to files (aka file path | ||
| strings) as necessary in order to reduce memory usage.""" | ||
| total_mem_size = CacheableImage.all_cacheable_images_size() | ||
| if total_mem_size <= memory_limit_bytes: |
There was a problem hiding this comment.
added this same comment to the code
| Note: this replaces the internal reference to source_path, if any, with | ||
| the newly given path. |
There was a problem hiding this comment.
Actually, going back in and expanding the documentation on this saved me from a potential bug! Thanks!
This PR is a biggie. It includes: