feat: intensitysensor #653

StefanThoene · 2025-07-03T13:55:02Z

Description

Create Sensor class

Issue linked

Checklist

I have tested my changes locally.
I have added necessary documentation or updated existing documentation.
I have followed the coding style guidelines of this project.
I have added appropriate tests (unit, integration, system).
I have reviewed my changes before submitting this pull request.
I have linked the issue or issues that are solved by the PR if any.
I have assigned this PR to myself.
I have made sure that the title of my PR follows Conventional commits style (e.g. feat: add optical property)
I have agreed with the Contributor License Agreement (CLA).

codecov · 2025-07-03T14:06:52Z

Codecov Report

Attention: Patch coverage is 25.17241% with 217 lines in your changes missing coverage. Please review.

Project coverage is 85.87%. Comparing base (b80e61e) to head (a8a5933).

Files with missing lines	Patch %	Lines
src/ansys/speos/core/sensor.py	25.17%	217 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
- Coverage   89.03%   85.87%   -3.16%     
==========================================
  Files          39       39              
  Lines        5581     5870     +289     
==========================================
+ Hits         4969     5041      +72     
- Misses        612      829     +217

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pluAtAnsys · 2025-07-03T19:23:00Z

src/ansys/speos/core/sensor.py

+            return False
+
+    @nearfield.setter
+    def nearfield(self, value):


if using property/setter methods, you will need to call the responding parameters setter methods as well.
i.e. for near field as true, then you need set call distance and cell diameter as well.
otherwise, the default setting will not happen.

src/ansys/speos/core/sensor.py

pluAtAnsys · 2025-07-03T19:52:53Z

src/ansys/speos/core/sensor.py

+            self.x_sampling = 180
+
+    @property
+    def x_start(self) -> float:


then x_start will be shown in the auto-completion methods list even the orientation is set as conoscopic.... I am not sure if this is good...

yes butt you can't set it and it will return none if the sensor is in conoscopic mode

src/ansys/speos/core/sensor.py

change from cell integration angle to cell diameter rename theta_max_sampling

add default values for nearfield sub elements

StefanThoene · 2025-07-10T07:10:52Z

src/ansys/speos/core/sensor.py

+        By default, ``0.3491``
+        """
+        if self.nearfield:
+            diameter = self.cell_distance * np.tan(


Suggested change

diameter = self.cell_distance * np.tan(

diameter = 2* self.cell_distance * np.tan(

i think that needs to be changed like that

echambla · 2025-07-11T12:18:02Z

src/ansys/speos/core/sensor.py

+            return False
+
+    @nearfield.setter
+    def nearfield(self, value):


Missing docstring :)

echambla · 2025-07-11T12:21:56Z

src/ansys/speos/core/sensor.py

+            raise TypeError("Sensor position is not in nearfield")
+
+    @property
+    def cell_diameter(self):


Do we want to keep the cell_diameter?
It implies to transform cell_integration_angle and cell_distance to get it. Whereas this kind of operation could be done by the customer.

The Speos Ui only has Celld iameter

echambla · 2025-07-11T12:23:25Z

src/ansys/speos/core/sensor.py

+        if type(self._type) is str:
+            return self._type
+        elif isinstance(self._type, BaseSensor.Colorimetric):


Suggested change

if type(self._type) is str:

return self._type

elif isinstance(self._type, BaseSensor.Colorimetric):

if isinstance(self._type, BaseSensor.Colorimetric):

(sugg) Because the else will do exactly same as the first if

echambla · 2025-07-11T12:26:03Z

src/ansys/speos/core/sensor.py

+            )
+        return self._type
+
+    def set_layer_type_none(self) -> SensorRadiance:


Suggested change

def set_layer_type_none(self) -> SensorRadiance:

def set_layer_type_none(self) -> SensorXMPIntensity:

echambla · 2025-07-11T12:28:08Z

src/ansys/speos/core/sensor.py

+        self,
+    ) -> Union[
+        None,
+        SensorIrradiance,


I think this is not correct, but I don't know why it is here.

Suggested change

SensorIrradiance,

echambla · 2025-07-11T12:28:46Z

src/ansys/speos/core/sensor.py

+        SensorIrradiance,
+        BaseSensor.LayerTypeFace,
+        BaseSensor.LayerTypeSequence,
+        BaseSensor.LayerTypeIncidenceAngle,


No LayerTypeIncidenceAngle for Intensity sensor

Suggested change

BaseSensor.LayerTypeIncidenceAngle,

echambla · 2025-07-11T12:29:14Z

src/ansys/speos/core/sensor.py

+            ansys.speos.core.sensor.SensorIrradiance,\
+            ansys.speos.core.sensor.BaseSensor.LayerTypeFace,\
+            ansys.speos.core.sensor.BaseSensor.LayerTypeSequence,\
+            ansys.speos.core.sensor.BaseSensor.LayerTypeIncidenceAngle\


Suggested change

ansys.speos.core.sensor.SensorIrradiance,\

ansys.speos.core.sensor.BaseSensor.LayerTypeFace,\

ansys.speos.core.sensor.BaseSensor.LayerTypeSequence,\

ansys.speos.core.sensor.BaseSensor.LayerTypeIncidenceAngle\

ansys.speos.core.sensor.BaseSensor.LayerTypeFace,\

ansys.speos.core.sensor.BaseSensor.LayerTypeSequence,\

echambla · 2025-07-11T12:29:57Z

src/ansys/speos/core/sensor.py

+        ]
+            Instance of Layertype Class for this sensor feature
+        """
+        return self._layer_type


Then what is returned when layer type is None, or Source?

I guess it could be harmonized like it is done for the type
-> property type
-> properties spectral and colorimetric

echambla · 2025-07-11T12:40:07Z

src/ansys/speos/core/sensor.py

+    def x_end(self) -> float:
+        """The maximum value on x-axis  (deg).
+
+        By default, ``45``.


I am wondering if a getter is the correct place to indicate default value.

echambla · 2025-07-11T12:49:02Z

@StefanThoene I see clearly how you want to use the properties.
Indeed all properties are available, and the user gets an error if he tries to use the wrong one depending on the current object.
I think this can work as well as the fluent api.

My only concern is the following:
As mentioned by Pengyuan, the user will have access to a huge number of properties, and will maybe be a bit lost to understand what can really be used or not.
Whereas the fluent api was guiding him step by step.

The decision is not only on my side.
Maybe it would be good to have some other AEs feedback, also @jomadec.

BUT, this is a really good feature to try this, you are right.
Maybe it would be good to complete with some basic examples so that we have a more specific idea about how it is used?

Thanks a lot for you work

SMoraisAnsys

Here are some comments. The comments on "return None" and "const variable" are echoed multiple times but I've stopped doing it and some cases are not highlighted by the review. Please have a look though :)

Concerning the use of property, I'm clearly approving this approach. As I said before, correct use of property improves readability and user experience. Speaking as a Python user, this is something common in Python packages and it is strongly leveraged in other PyAnsys packages. @echambla If there is a reason to focus on fluent API please contact me so that you can clarify that topic to me

SMoraisAnsys · 2025-07-16T06:51:29Z

src/ansys/speos/core/sensor.py

+        """Property containing if the sensor is positioned in nearfield or infinity."""
+        if self._sensor_template.intensity_sensor_template.HasField("near_field"):
+            return True
+        else:
+            return False


Suggested change

"""Property containing if the sensor is positioned in nearfield or infinity."""

if self._sensor_template.intensity_sensor_template.HasField("near_field"):

return True

else:

return False

"""Return True if the sensor is positioned in nearfield, otherwise False."""

return self._sensor_template.intensity_sensor_template.HasField("near_field")

I'm guessing that HasField returns a boolean. If not, ignore this comment :)

SMoraisAnsys · 2025-07-16T06:53:50Z

src/ansys/speos/core/sensor.py

+                self.cell_distance = 10
+                self.cell_diameter = 0.3491


Could you define CELL_DISTANCE and CELL_DIAMETER at the beginning of the file and leverage it in the file (both code and docs) ? That will help you with maintenance and testing since changing the value in a single place will be applyed everywhere the "constant" variable is imported and used.

SMoraisAnsys · 2025-07-16T06:58:31Z

src/ansys/speos/core/sensor.py

+                self.cell_diameter = 0.3491
+        else:
+            if self._sensor_template.intensity_sensor_template.HasField("near_field"):
+                self._sensor_template.intensity_sensor_template.ClearField("near_field")


Would it be worth to warn your user if HasField("near_field") is False ? Otherwise you can just use elif right ?

SMoraisAnsys · 2025-07-16T06:58:46Z

src/ansys/speos/core/sensor.py

+    def cell_distance(self):
+        """Distance of the Detector to origin in mm.
+
+        By default, ``10``


cf const var

SMoraisAnsys · 2025-07-16T06:59:27Z

src/ansys/speos/core/sensor.py

+        if self.nearfield:
+            return self._sensor_template.intensity_sensor_template.near_field.cell_distance
+        else:
+            return None


Suggested change

if self.nearfield:

return self._sensor_template.intensity_sensor_template.near_field.cell_distance

else:

return None

if self.nearfield:

return self._sensor_template.intensity_sensor_template.near_field.cell_distance

Not sure the else is needed. By default python returns None if nothing is returned

SMoraisAnsys · 2025-07-16T07:05:04Z

src/ansys/speos/core/sensor.py

+        self._set_default_dimension_values()
+
+    def set_viewing_direction_from_source(self):
+        """Set viewing direction from Source Looking At Sensor."""


Suggested change

"""Set viewing direction from Source Looking At Sensor."""

"""Set viewing direction from source looking at sensor."""

SMoraisAnsys · 2025-07-16T07:05:28Z

src/ansys/speos/core/sensor.py

+        self._sensor_template.intensity_sensor_template.from_source_looking_at_sensor.SetInParent()
+
+    def set_viewing_direction_from_sensor(self):
+        """Set viewing direction from Sensor Looking At Source."""


Suggested change

"""Set viewing direction from Sensor Looking At Source."""

"""Set viewing direction from sensor looking at source."""

SMoraisAnsys · 2025-07-16T07:06:00Z

src/ansys/speos/core/sensor.py

+    def x_start(self) -> float:
+        """The minimum value on x-axis  (deg).
+
+        By default, ``45``.


echo const var

SMoraisAnsys · 2025-07-16T07:11:15Z

src/ansys/speos/core/sensor.py

+        """
+        template = self._sensor_template.intensity_sensor_template
+        if template.HasField("intensity_orientation_conoscopic"):
+            return None


Can you log a warning here if that's not something that should be used with conoscopic ?

@pluAtAnsys If you really want to avoid that kind of behavior, you could create specific classes for x_as_parallel and conoscopic ? Each would inherit from SensorXMPIntensity and have a subset of dedicated properties. Note that this would start to be verbose if you want to move your sensor from X to Conoscopic.

SMoraisAnsys · 2025-07-16T07:18:00Z

src/ansys/speos/core/sensor.py

+        else:
+            raise TypeError("Only Conoscopic Sensor has theta_max dimension")
+
+    def set_type_photometric(self) -> SensorXMPIntensity:


Just wondering if set_type_photometric, set_type_colorimetric, set_type_radiometric, set_type_spectral, set_layer_type_none, set_layer_type_source, set_layer_type_face and set_layer_type_sequence should be hidden or be homogenized ? Some of them return self other return self._type.
If possible I would hidde them if that can be set in the init through some parameter and if this isn't supposed to change while the object is alive.

StefanThoene added 3 commits July 3, 2025 13:32

implement first draft

8cd52c4

implement first draft

34c009f

improve to prototype

55274ad

github-actions bot added the enhancement New features or code improvements label Jul 3, 2025

pyansys-ci-bot and others added 2 commits July 3, 2025 13:56

chore: adding changelog file 653.added.md [dependabot-skip]

682ed1d

Merge branch 'main' into feat/Intensitysensor

15004fb

pluAtAnsys requested changes Jul 3, 2025

View reviewed changes

pluAtAnsys assigned StefanThoene Jul 3, 2025

pluAtAnsys linked an issue Jul 3, 2025 that may be closed by this pull request

Add intensity sensor #654

Open

pluAtAnsys and others added 3 commits July 4, 2025 09:50

Merge branch 'main' into feat/Intensitysensor

661ebb3

convert axissystem to property

305b3cd

change from cell integration angle to cell diameter rename theta_max_sampling

fix sim export_unittest for windows

2b22966

add default values for nearfield sub elements

github-actions bot added the testing Anything related to tests label Jul 4, 2025

StefanThoene and others added 4 commits July 9, 2025 13:52

Merge branch 'main' into feat/Intensitysensor

536889a

Merge branch 'main' into feat/Intensitysensor

8abb8dc

Fix issue with import

a5695da

remove math import

a8a5933

StefanThoene commented Jul 10, 2025

View reviewed changes

echambla reviewed Jul 11, 2025

View reviewed changes

SMoraisAnsys reviewed Jul 16, 2025

View reviewed changes

	diameter = self.cell_distance * np.tan(
	diameter = 2* self.cell_distance * np.tan(

	def set_layer_type_none(self) -> SensorRadiance:
	def set_layer_type_none(self) -> SensorXMPIntensity:

	"""Set viewing direction from Source Looking At Sensor."""
	"""Set viewing direction from source looking at sensor."""

	"""Set viewing direction from Sensor Looking At Source."""
	"""Set viewing direction from sensor looking at source."""

feat: intensitysensor #653

Are you sure you want to change the base?

feat: intensitysensor #653

Uh oh!

Conversation

StefanThoene commented Jul 3, 2025 • edited by pluAtAnsys Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue linked

Checklist

Uh oh!

codecov bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pluAtAnsys Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanThoene Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echambla commented Jul 11, 2025

Uh oh!

SMoraisAnsys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

StefanThoene commented Jul 3, 2025 •

edited by pluAtAnsys

Loading

codecov bot commented Jul 3, 2025 •

edited

Loading

pluAtAnsys Jul 3, 2025 •

edited

Loading

StefanThoene Jul 4, 2025 •

edited

Loading