82 spot analysis for peak intensity correction #87

bbean23 · 2024-04-17T02:11:30Z

Note: this ticket got unwieldly big. Splitting between here and #91

Add spot analysis code to analyze peak intensities of a heliostat. The necessary features include:

Additional features that might be helpful include:

directory watcher image source, to introduce new images to the pipeline as they are added to the source directory
image translation correction based on tower fiducials
pixel intensity to actual flux correction [s]
image scale tagging (how many meters is a pixel?)

bbean23 · 2024-04-24T17:38:49Z

@e10harvey @braden6521 I added both of you as reviewers in case one of you really wants to do this review, not because I think you both need to do it. I suggest adding a comment that you're claiming the review.

e10harvey · 2024-04-25T13:18:54Z

@e10harvey @braden6521 I added both of you as reviewers in case one of you really wants to do this review, not because I think you both need to do it. I suggest adding a comment that you're claiming the review.

Thank you, @bbean23. This is a very large diff. It is impossible for me to review this. Can you split this into multiple PRs in a logical manner. Perhaps on a per-class or per-related tests basis. It would also be helpful to the reviewers to provide a summary of the changes in the PR description. The longer this PR sits, the harder it is going to be merge this in. Please to to rebase on-top of develop on a weekly basis to avoid high cognitive load when resolving conflicts.

bbean23 · 2024-04-25T16:44:54Z

Thank you, @bbean23. This is a very large diff. It is impossible for me to review this. Can you split this into multiple PRs in a logical manner. Perhaps on a per-class or per-related tests basis. It would also be helpful to the reviewers to provide a summary of the changes in the PR description. The longer this PR sits, the harder it is going to be merge this in. Please to to rebase on-top of develop on a weekly basis to avoid high cognitive load when resolving conflicts.

Sorry about this PR being so big. As you can see by the description, I actually started this PR because I realized that it was becoming so big in the first place.

The summary of changes was on the ticket, and I've now copied that summary to here.
Braden let me know that he started reviewing this. I've asked him if it will be helpful for me to split it into multiple pieces.
Your comment about rebasing it frequently is very insightful and well received. I will absolutely do this.

braden6521

@bbean23, some of this was over my head to be honest. But the code looks well written and well documented. I have some simple suggestions, but feel free to take or leave them.

braden6521 · 2024-04-24T23:20:15Z

opencsp/common/lib/cv/AbstractFiducials.py

+        pixels_to_meters : Callable[[p2.Pxy], v3.Vxyz], optional
+            Conversion function to get the physical point in space for the given x/y position information. Used in the
+            default self.scale implementation. Defaults to 1 meter per pixel.


Some questions, although this is not necessarily wrong:

Does this assume that the camera is normal to the fiducials?

Is distortion taken into account?

Will different pixels ever have different scales?

These are really good questions, and I'm glad that you brought them up. I've updated the docstring to try and answer them:

""" pixels_to_meters : Callable[[p2.Pxy], v3.Vxyz], optional Conversion function to get the physical point in space for the given x/y position information. Used in the default self.scale implementation. A good implementation of this function will correct for many factors such as relative camera position and camera distortion. For extreme accuracy, this will also account for non-uniformity in the target surface. Defaults to a simple 1 meter per pixel model. """

braden6521 · 2024-04-24T23:21:41Z

contrib/app/SpotAnalysis/PeakFlux.py

I don't think I have the background to fully understand how this works. But generally, this code looks good.

Thanks! I don't think you're missing a background, you're just not in my head. :)

In reality, understanding this class requires understanding the SpotAnalysis class, which will hopefully get easier to do with some examples.

braden6521 · 2024-04-24T23:24:27Z

opencsp/common/lib/cv/AbstractFiducials.py

+class AbstractFiducials(ABC):
+    def __init__(self, style=None, pixels_to_meters: Callable[[p2.Pxy], v3.Vxyz] = None):
+        """
+        A collection of markers (such as an ArUco board) that is used to orient the camera relative to observed objects
+        in the scene. It is suggested that each implementing class be paired with a complementary locator method or
+        SpotAnalysisImageProcessor.
+
+        Parameters
+        ----------
+        style : RenderControlPointSeq, optional
+            How to render this fiducial when using the defaul render_to_plot() method. By default rcps.default().
+        pixels_to_meters : Callable[[p2.Pxy], v3.Vxyz], optional
+            Conversion function to get the physical point in space for the given x/y position information. Used in the
+            default self.scale implementation. Defaults to 1 meter per pixel.
+        """


We should probably get in the habit of adding docstrings to all our classes just so it's easier to add these new classes to the docstring test later. Feel free to do this at a later date too. I noticed a few classes with no docstrings here but didn't point them all out.

Suggested change

class AbstractFiducials(ABC):

def __init__(self, style=None, pixels_to_meters: Callable[[p2.Pxy], v3.Vxyz] = None):

"""

A collection of markers (such as an ArUco board) that is used to orient the camera relative to observed objects

in the scene. It is suggested that each implementing class be paired with a complementary locator method or

SpotAnalysisImageProcessor.

Parameters

----------

style : RenderControlPointSeq, optional

How to render this fiducial when using the defaul render_to_plot() method. By default rcps.default().

pixels_to_meters : Callable[[p2.Pxy], v3.Vxyz], optional

Conversion function to get the physical point in space for the given x/y position information. Used in the

default self.scale implementation. Defaults to 1 meter per pixel.

"""

class AbstractFiducials(ABC):

"""

A collection of markers (such as an ArUco board) that is used to orient the camera relative to observed objects

in the scene. It is suggested that each implementing class be paired with a complementary locator method or

SpotAnalysisImageProcessor.

Parameters

----------

style : RenderControlPointSeq, optional

How to render this fiducial when using the defaul render_to_plot() method. By default rcps.default().

pixels_to_meters : Callable[[p2.Pxy], v3.Vxyz], optional

Conversion function to get the physical point in space for the given x/y position information. Used in the

default self.scale implementation. Defaults to 1 meter per pixel.

"""

def __init__(self, style=None, pixels_to_meters: Callable[[p2.Pxy], v3.Vxyz] = None):

I wasn't sure where to put these docstrings, since they seem like they belong both on the class description and on the init. It looks like realpython suggests splitting it, putting the description on the class and the parameters on the method. I've updated the code to follow that approach. What do you think?

https://realpython.com/documenting-python-code/#class-docstrings

Yes, that sounds like a good idea! I like that standard.

braden6521 · 2024-04-24T23:45:27Z

opencsp/common/lib/cv/fiducials/BcsFiducial.py

+    @property
+    def orientation(self) -> v3.Vxyz:
+        return v3.Vxyz([0, 0, 0])


Is this "orientation" supposed to have a rotation component too, or is this a pointing direction?

It's intended to be an absolute orientation in relation to the camera. Do we also need a pointing direction for that?

If you want a full six degree of freedom orientation, then you would need a rotation and translation, but I'm not sure that's what you're trying to do here.

braden6521 · 2024-04-24T23:49:28Z

opencsp/common/lib/cv/AbstractFiducials.py

+    def orientation(self) -> v3.Vxyz:
+        """The orientation(s) of this instance, in radians. This is relative to
+        the source image, where x is positive to the right, y is positive down,
+        and z is positive in (away from the camera)."""


Is this supposed to be "radians?" it looks like it outputs a 3d vector? If it is radians, did you consider using a scipy.spatial.transform.Rotation object?

That is a brilliant idea! Thanks! 🧠

If you don't mind, please take a look at the updated description and let me know what you think 2568e2d:

""" The orientation of the normal vector(s) of this instance. This is relative to the orthorectified source image, where x is positive to the right, y is positive down, and z is positive in (away from the camera). This can be used to describe the forward transformation from the camera's perspective. For example, an aruco marker whose origin is in the center of the image and is facing towards the camera could have the orientation:: Rotation.from_euler('y', np.pi) If that same aruco marker was also placed upside down, then it's orientation could be:: Rotation.from_euler( 'yz', [ [np.pi, 0], [0, np.pi] ] ) """

I think that description is overall good! I'm not sure I understand what is meant by:

relative to the orthorectified source image

Is this the image on the camera sensor? If so, why is this orthorectified? If it is the camera or the target coordinates, I would just expect "camera" or "target."

Also I would add that this is just a rotation, not translation. When I think of an orientation, I usually think of rotation and translation (six degree of freedom (or six DOF)). Maybe consider renaming this rotation?

opencsp/common/lib/cv/spot_analysis/image_processor/SupportingImagesCollectorImageProcessor.py

braden6521 · 2024-04-26T15:46:40Z

opencsp/common/lib/cv/spot_analysis/image_processor/__init__.py

This is nice

Thanks! It's really handy for this sort of situation where:

you're going to be using lots of random classes

they all have the same name so it's easy to tell which package they came from

opencsp/common/lib/geometry/Vxy.py

opencsp/common/lib/geometry/Vxyz.py

opencsp/common/lib/tool/file_tools.py

bbean23

Thank you for your thoughtful and considerate review, as always! I've made some commits to try and improve the code based on your comments. Please let me know what you think of these.

bbean23 · 2024-04-26T19:52:45Z

contrib/app/SpotAnalysis/PeakFlux.py

Thanks! I don't think you're missing a background, you're just not in my head. :)

In reality, understanding this class requires understanding the SpotAnalysis class, which will hopefully get easier to do with some examples.

bbean23 · 2024-04-26T19:54:15Z

opencsp/common/lib/cv/AbstractFiducials.py

+        pixels_to_meters : Callable[[p2.Pxy], v3.Vxyz], optional
+            Conversion function to get the physical point in space for the given x/y position information. Used in the
+            default self.scale implementation. Defaults to 1 meter per pixel.


These are really good questions, and I'm glad that you brought them up. I've updated the docstring to try and answer them:

""" pixels_to_meters : Callable[[p2.Pxy], v3.Vxyz], optional Conversion function to get the physical point in space for the given x/y position information. Used in the default self.scale implementation. A good implementation of this function will correct for many factors such as relative camera position and camera distortion. For extreme accuracy, this will also account for non-uniformity in the target surface. Defaults to a simple 1 meter per pixel model. """

bbean23 · 2024-04-26T20:13:30Z

opencsp/common/lib/cv/AbstractFiducials.py

+class AbstractFiducials(ABC):
+    def __init__(self, style=None, pixels_to_meters: Callable[[p2.Pxy], v3.Vxyz] = None):
+        """
+        A collection of markers (such as an ArUco board) that is used to orient the camera relative to observed objects
+        in the scene. It is suggested that each implementing class be paired with a complementary locator method or
+        SpotAnalysisImageProcessor.
+
+        Parameters
+        ----------
+        style : RenderControlPointSeq, optional
+            How to render this fiducial when using the defaul render_to_plot() method. By default rcps.default().
+        pixels_to_meters : Callable[[p2.Pxy], v3.Vxyz], optional
+            Conversion function to get the physical point in space for the given x/y position information. Used in the
+            default self.scale implementation. Defaults to 1 meter per pixel.
+        """


I wasn't sure where to put these docstrings, since they seem like they belong both on the class description and on the init. It looks like realpython suggests splitting it, putting the description on the class and the parameters on the method. I've updated the code to follow that approach. What do you think?

https://realpython.com/documenting-python-code/#class-docstrings

opencsp/common/lib/geometry/Vxy.py

opencsp/common/lib/geometry/Vxyz.py

bbean23 · 2024-04-26T20:43:59Z

opencsp/common/lib/cv/spot_analysis/image_processor/NullImageSubtractionImageProcessor.py

+
+class NullImageSubtractionImageProcessor(AbstractSpotAnalysisImagesProcessor):
+    """
+    Subtracts the NULL supporting image from the primary image, if there is an associated NULL image.


It's an image of a target without a beam on it. (Thanks for prompting me to add descriptions 62e77ae)

I wasn't sure what to call this type of image. Do you have a good idea?

opencsp/common/lib/cv/spot_analysis/image_processor/AnnotationImageProcessor.py

bbean23 · 2024-04-26T20:49:22Z

opencsp/common/lib/cv/fiducials/PointFiducials.py

+    @property
+    def orientation(self) -> v3.Vxyz:
+        # TODO untested
+        return np.zeros((3, self.points.x.size))


I do like that more. Done!

bbean23 · 2024-04-26T21:15:11Z

opencsp/common/lib/cv/fiducials/BcsFiducial.py

+    @property
+    def orientation(self) -> v3.Vxyz:
+        return v3.Vxyz([0, 0, 0])


It's intended to be an absolute orientation in relation to the camera. Do we also need a pointing direction for that?

bbean23 · 2024-04-26T21:16:31Z

opencsp/common/lib/cv/AbstractFiducials.py

+    def orientation(self) -> v3.Vxyz:
+        """The orientation(s) of this instance, in radians. This is relative to
+        the source image, where x is positive to the right, y is positive down,
+        and z is positive in (away from the camera)."""


That is a brilliant idea! Thanks! 🧠

If you don't mind, please take a look at the updated description and let me know what you think 2568e2d:

""" The orientation of the normal vector(s) of this instance. This is relative to the orthorectified source image, where x is positive to the right, y is positive down, and z is positive in (away from the camera). This can be used to describe the forward transformation from the camera's perspective. For example, an aruco marker whose origin is in the center of the image and is facing towards the camera could have the orientation:: Rotation.from_euler('y', np.pi) If that same aruco marker was also placed upside down, then it's orientation could be:: Rotation.from_euler( 'yz', [ [np.pi, 0], [0, np.pi] ] ) """

braden6521

@bbean23, thanks for answering my questions. Feel free to take or leave my new comments. I still need some more exposure to all this to fully wrap my head around it. I tried to do my best with this large PR, but in general the code looks good.

…dialabs#87

e10harvey · 2024-05-29T15:57:24Z

@bbean23: I restarted the windows2022 CI.

e10harvey · 2024-05-29T16:20:11Z

@bbean23: I believe restarting is using the previous version of develop, sans the commits from #95. Please rebase this on-top of develop.

…dialabs#87

bbean23 · 2024-05-29T16:48:51Z

@bbean23: I believe restarting is using the previous version of develop, sans the commits from #95. Please rebase this on-top of develop.

I think you're right about this. I just rebased and pushed. I'll ping you again to approve once the builds are finished.

braden6521

Approved!

…" example, expand SpotAnalysis desired features list

…tAnalysisOperable

…bleImage

… add initial_min and initial_max to PopulationStatisticsImageProcessor

…matting fixes

…ure of the Pxy and Vxyz classes

…essor

…dialabs#87

bbean23 force-pushed the 82-spot-analysis-for-peak-intensity-correction branch 2 times, most recently from a0512b8 to 8da78ac Compare April 24, 2024 17:35

bbean23 requested review from e10harvey and braden6521 April 24, 2024 17:36

bbean23 self-assigned this Apr 24, 2024

bbean23 added the enhancement New feature or request label Apr 24, 2024

bbean23 linked an issue Apr 24, 2024 that may be closed by this pull request

spot analysis for peak intensity correction #82

Closed

14 tasks

bbean23 marked this pull request as ready for review April 24, 2024 17:46

braden6521 previously approved these changes Apr 26, 2024

View reviewed changes

bbean23 dismissed braden6521’s stale review via 13fa589 April 26, 2024 20:33

bbean23 force-pushed the 82-spot-analysis-for-peak-intensity-correction branch 2 times, most recently from 1a35c4a to 2568e2d Compare April 26, 2024 21:13

bbean23 commented Apr 26, 2024

View reviewed changes

bbean23 mentioned this pull request May 1, 2024

3d visualization of spot #97

Merged

braden6521 previously approved these changes May 20, 2024

View reviewed changes

bbean23 added a commit to bbean23/OpenCSP that referenced this pull request May 24, 2024

better method names and descriptions, to match the comments on PR san…

4341257

…dialabs#87

bbean23 dismissed braden6521’s stale review via 4341257 May 24, 2024 22:08

bbean23 force-pushed the 82-spot-analysis-for-peak-intensity-correction branch from 0e193f2 to 4341257 Compare May 24, 2024 22:08

braden6521 mentioned this pull request May 29, 2024

Failing Unit Tests in TestMirrorOutput and TestSolarFieldOutput #109

Closed

bbean23 added a commit to bbean23/OpenCSP that referenced this pull request May 29, 2024

better method names and descriptions, to match the comments on PR san…

e2d5055

…dialabs#87

bbean23 force-pushed the 82-spot-analysis-for-peak-intensity-correction branch from 4341257 to e2d5055 Compare May 29, 2024 16:48

braden6521 approved these changes May 29, 2024

View reviewed changes

bbean23 added 2 commits May 29, 2024 15:36

add collaborative_dir setting, fix SpotAnalysis __name__ == "__main__…

7f7cedc

…" example, expand SpotAnalysis desired features list

start sandialabs#82 spot analysis for peak intensity correction

7f1e53b

bbean23 added 27 commits May 29, 2024 15:36

formatting

5f5a10c

add ExposureDetectionImageProcessor, add image_processor_notes to Spo…

5fd2559

…tAnalysisOperable

add TestExposureDetectionImageProcessor

56f5498

add AbstractAggregateImageProcessor and associated test

d8fe94b

add AverageByGroupImageProcessor and associated unit test, fix Cachea…

4c14072

…bleImage

add write_json and read_json to file_tools.py

97d55eb

more prototyping of the PeakFlux workflow

04ba15d

fix AbstractAggregateImageProcessor and AverageByGroupImageProcessor,…

4237aa4

… add initial_min and initial_max to PopulationStatisticsImageProcessor

fix SpotAnalysisImageAttributeParser by fixing ImageAttributeParser

a5ee42b

more debugging options

4a5841f

add SupportingImagesCollectorImageProcessor, other small code and for…

60093db

…matting fixes

add NullImageSubtractionImageProcessor.py

ad46e21

better comments in CacheableImage

de9f978

add ConvolutionImageProcessor

5b0c73d

update AbstractFiducial->AbstractFiducials to indicate the plural nat…

5c011b7

…ure of the Pxy and Vxyz classes

fix SpotAnalysisOperable

f7ac1fa

add AnnotationImageProcessor, add PointFiducials

eb141e5

remove unused imports, fix test file location

2b7ad94

add BcsFiducial, BcsLocatorImageProcessor, RenderControlBcs

bd484e3

remove unused code from PeakFlux and CroppingImageProcessor

da10f68

add addition comments

66631a9

updating docstrings as suggested in PR

0a7a5e5

better comments and code order for SupportingImagesCollectorImageProc…

920663b

…essor

add descriptions for the various ImageTypes

a6d74a5

better AbstractFiducial.orientation description

c75a80b

formatting

c80b85b

better method names and descriptions, to match the comments on PR san…

918a09b

…dialabs#87

bbean23 force-pushed the 82-spot-analysis-for-peak-intensity-correction branch from e2d5055 to 918a09b Compare May 29, 2024 21:36

bbean23 merged commit e432934 into sandialabs:develop May 29, 2024
4 checks passed

bbean23 mentioned this pull request Jun 3, 2024

spot analysis for peak intensity correction #82

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

82 spot analysis for peak intensity correction #87

82 spot analysis for peak intensity correction #87

bbean23 commented Apr 17, 2024 •

edited

Loading

bbean23 commented Apr 24, 2024

e10harvey commented Apr 25, 2024

bbean23 commented Apr 25, 2024

braden6521 left a comment

braden6521 Apr 24, 2024

bbean23 Apr 26, 2024

braden6521 Apr 24, 2024

bbean23 Apr 26, 2024

braden6521 Apr 24, 2024

bbean23 Apr 26, 2024

braden6521 May 29, 2024

braden6521 Apr 24, 2024

bbean23 Apr 26, 2024

braden6521 May 20, 2024

braden6521 Apr 24, 2024

bbean23 Apr 26, 2024

braden6521 May 20, 2024 •

edited

Loading

braden6521 Apr 26, 2024

bbean23 Apr 26, 2024

bbean23 left a comment

bbean23 Apr 26, 2024

bbean23 Apr 26, 2024

bbean23 Apr 26, 2024

bbean23 Apr 26, 2024

bbean23 Apr 26, 2024

bbean23 Apr 26, 2024

bbean23 Apr 26, 2024

braden6521 left a comment

e10harvey commented May 29, 2024

e10harvey commented May 29, 2024

bbean23 commented May 29, 2024

braden6521 left a comment

82 spot analysis for peak intensity correction #87

82 spot analysis for peak intensity correction #87

Conversation

bbean23 commented Apr 17, 2024 • edited Loading

bbean23 commented Apr 24, 2024

e10harvey commented Apr 25, 2024

bbean23 commented Apr 25, 2024

braden6521 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

braden6521 May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

braden6521 left a comment

Choose a reason for hiding this comment

e10harvey commented May 29, 2024

e10harvey commented May 29, 2024

bbean23 commented May 29, 2024

braden6521 left a comment

Choose a reason for hiding this comment

bbean23 commented Apr 17, 2024 •

edited

Loading

braden6521 May 20, 2024 •

edited

Loading