add solar calculation in weather_controller and define fenestration constants by ecosang · Pull Request #165 · google/sbsim

ecosang · 2026-03-16T08:12:33Z

add solar calculation in weather_controller and define fenestration constants

…onstants, and add relative convergence argument in simulator

ecosang · 2026-03-16T08:12:54Z

@s2t2 I've put only weather controller part in this PR.

s2t2 · 2026-03-19T14:16:04Z

OK, thanks! I see the build is failing due to CSV path issues (see below). Would you please fix and then ping me and I will review again when the build is passing.

FAILED smart_control/simulator/weather_controller_test.py::ReplayWeatherControllerPvlibValidationTest::test_replay_controller_irradiance_closure_equation - FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/work/sbsim/sbsim/smart_control/simulator/../configs/resources/sb1/weather_data/local_weather_test_data.csv'

ecosang · 2026-03-19T20:34:34Z

@s2t2 the csv file is automatically ignored. I just added. Sorry for the iteration. Can I also check if the testing is passed here on my end? when I made PR, I didn't see the failure. Maybe I didn't wait enough.

s2t2 · 2026-03-23T19:11:25Z

For some reason, I have to click the "Approve workflows and run" button to trigger the tests. Running now...

s2t2

Hi @ecosang thanks for this PR.

I left some comments below.

One of the major ones is to see if we can use separate test classes to leverage shared set up methods (as applicable), and see if we can use test class inheritance to reduce the amount of test code (if possible).

Also it would be helpful to define some of the expected column names and sensor names as constants.

Also we can see if using some dataclasses would be helpful in specific situations mentioned.

There are some opportunities to refactor shared methods.

Also, let's discuss how to best handle the pvlib dependency, either making it non-optional, or implementing it as optional using the pyproject.toml / poetry mechanism used for this purpose.

s2t2 · 2026-03-25T14:38:45Z

smart_control/simulator/building_radiation_test_data/local_weather_test_data.csv

There is an existing test data file in "smart_control/simulator/local_weather_test_data.csv". I was wondering how these files differ, and if we could leverage the existing file. Or organize both of these files to be in the same place (perhaps putting them in a "smart_control/simulator/test_data" folder.)

Oh I see you moved this file from that location, to be in the resources dir. That could be OK. However I would prefer to keep both test data files in the same location.

I put this in "building_radiation_test_data" folder.

Since the local weather test data is used for replay weather controller tests (without radiation), let's prefer to keep it where it is. Or perhaps we can move both CSV files to a generic "simulator/test_data" directory.

s2t2 · 2026-03-25T14:40:02Z

smart_control/simulator/building_radiation_test_data/723170TYA.CSV

It would be nice to know how this data was acquired, what the source is, and perhaps also to have access to the code that pulled this data (as applicable).

s2t2 · 2026-03-25T14:44:27Z

smart_control/simulator/building_radiation_utils.py

+    solar_azimuth: Solar azimuth angle in degrees (compass direction of sun).
+  Returns:
+    POA global irradiance in W/m2.
+  Example:


Appreciate the example in the docstring.

s2t2 · 2026-03-25T14:50:47Z

smart_control/simulator/building_radiation_utils.py

+    ...     surface_azimuth=180.0, solar_zenith=30.0, solar_azimuth=180.0)
+  """
+  # Import here to avoid circular imports and keep pvlib as optional dependency
+  from pvlib import irradiance as pvlib_irradiance  # pylint: disable=import-outside-toplevel


Can you please further explain the circular dependency concerns if this import were placed at the top of the file? And also separately some reasons we would want to keep pvlib as an optional dependency?

FYI there is a way we can do optional dependencies using pyproject.toml, like for example to great a new group called "solar":

[tool.poetry.group.docs.solar] pvlib = "0.13.1"

Then it can be installed like poetry install --with solar (see Setup docs related to notebook setup, for an example.

i updated it.. 3bf35f5#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R62

also the lock file.

3bf35f5#diff-f53a023eedfa3fbf2925ec7dc76eecdc954ea94b7e47065393dbad519613dc89R1

Not sure if it is what you suggested.

Let's just do a normal import of the pvlib at the top of the file.

It is a small enough dependency, and we are already using scipy, pandas, numpy, etc, so size isn't a big concern.

I know we discussed possibly making it an optional dependency in the poetry / pyproject.toml, but I don't see a strong argument for that, unless there's something I'm missing.

s2t2 · 2026-03-25T14:52:43Z

smart_control/simulator/building_radiation_utils.py

+  Converts horizontal irradiance components (GHI, DNI, DHI) to the irradiance
+  incident on a tilted surface. Uses the pvlib library's get_total_irradiance
+  function.
+  Args:


Nit: I believe we might need a new line before each of these sections in the docstring: Args, Returns, Example. And I believe Return should go last.

s2t2 · 2026-03-25T16:04:33Z

smart_control/simulator/weather_controller.py

+    Uses Clark & Allen formula with dry bulb and dew point temperatures.
+    """
+    # Stefan-Boltzmann constant
+    sigma = 5.6697e-8  # W/(m^2*K^4)


I see this constant is used in building radiation utils file as well. Let's define it once in the simulation/constants.py file, and import / access from there.

sbsim/smart_control/simulator/building_radiation_utils.py

Line 158 in 4f32db2

sigma = 5.67 * 1e-8 # [W/m^2K^4] Stefan-Boltzmann constant

265540f#diff-ba4316e0f4a2ac3741564675aad1c132fbac964047a1f416d1adb4e1c59175acR177

s2t2 · 2026-03-25T16:06:24Z

smart_control/simulator/weather_controller.py

+
+    # Get dry bulb temperature in Kelvin (use numpy operations for arrays)
+    if 'TempC' in self._weather_data:
+      temp_k = self._weather_data['TempC'].values + 273.15


if possible, let's use / apply temp conversion functions from conversion utils (c to k, f to k)

applies for temp and dewpoint below

265540f#diff-256b9901b7e7079817a790409a91e62dc0c8797bc5bef0b390390652bb82c41aR706

265540f#diff-256b9901b7e7079817a790409a91e62dc0c8797bc5bef0b390390652bb82c41aR718

265540f#diff-256b9901b7e7079817a790409a91e62dc0c8797bc5bef0b390390652bb82c41aR742

s2t2 · 2026-03-25T16:14:04Z

smart_control/simulator/weather_controller.py

+  # Stefan-Boltzmann constant
+  sigma = 5.6697e-8  # W/(m^2*K^4)
+
+  def get_value(observation_response, measurement_name):


I see these nested get_value methods are repeated. Let's prefer to move them to a single function at the top of the file (outside the class), and invoke from that location.

265540f#diff-256b9901b7e7079817a790409a91e62dc0c8797bc5bef0b390390652bb82c41aR465

s2t2 · 2026-03-25T16:14:39Z

smart_control/simulator/weather_controller.py

+      continue
+
+    # Try to get dew point temperature, otherwise estimate from depression
+    dp_k = get_value(r, 'dew_point_temperature_sensor')


Let's define these sensor names as constants at the top of the file.

it is done in 265540f#diff-256b9901b7e7079817a790409a91e62dc0c8797bc5bef0b390390652bb82c41aR29

s2t2 · 2026-03-25T16:19:38Z

smart_control/simulator/weather_controller.py

+    )
+    return float(cloud_cover)
+
+  def get_current_irradiance(


If the specific structure of a dictionary containing these specific keys is used in many places, let's consider using a dataclass to define those expected properties. That would also allow us to better document the meaning behind each of these properties.

@dataclasses.dataclass class Irradiance """ Docstring with ... Args: ... """ ghi: float dni: float dhi: float # etc.

This function can then return an instance of that dataclass.

Later, when we need to reference the values, we can use dot notation (like irr.ghi) or convert the dataclass to a dict using dataclasses.asdict() and access the values using dictionary notation.

I added it in 265540f#diff-0cfd48097b60803cd9e61fff1f3aa765d176d8394319cd0a2eaf449ff32f2dcdR171

…, also move testing csv files into raditaion util folder

s2t2

Hi @ecosang thanks for the updates. I provided another round of comments.

FYI - in a few comments I am just testing to see if Gemini Code Assist is installed correctly.

s2t2 · 2026-03-31T14:01:26Z

smart_control/simulator/building_radiation_test_data/local_weather_test_data.csv

Since the local weather test data is used for replay weather controller tests (without radiation), let's prefer to keep it where it is. Or perhaps we can move both CSV files to a generic "simulator/test_data" directory.

s2t2 · 2026-03-31T14:02:46Z

smart_control/simulator/building_radiation_test_data/station.json

@@ -0,0 +1,12 @@
+{


Can we use the JSON file from the configs/resources directory, instead of creating a copy of it here? That way we have one source of truth.

https://github.com/google/sbsim/blob/copybara_push/smart_control/configs/resources/sb1/weather_data/station.json

The main reason is station depends on the weather file. So, csv file should be with the station.json.

s2t2 · 2026-03-31T14:04:17Z

smart_control/simulator/building_radiation_utils.py

 from smart_control.simulator import constants

+if TYPE_CHECKING:
+  from smart_control.simulator import weather_controller


Let's do a normal import here.

s2t2 · 2026-03-31T14:04:47Z

smart_control/simulator/building_radiation_utils.py

+
+
+def calculate_poa_irradiance(
+    irradiance_components: 'weather_controller.IrradianceComponents',


Let's use the actual class here for type hint

s2t2 · 2026-03-31T14:07:14Z

smart_control/simulator/building_radiation_utils.py

+    ...     surface_azimuth=180.0, solar_zenith=30.0, solar_azimuth=180.0)
+  """
+  # Import here to avoid circular imports and keep pvlib as optional dependency
+  from pvlib import irradiance as pvlib_irradiance  # pylint: disable=import-outside-toplevel


Let's just do a normal import of the pvlib at the top of the file.

It is a small enough dependency, and we are already using scipy, pandas, numpy, etc, so size isn't a big concern.

I know we discussed possibly making it an optional dependency in the poetry / pyproject.toml, but I don't see a strong argument for that, unless there's something I'm missing.

s2t2 · 2026-03-31T14:15:56Z

smart_control/simulator/simulator.py

      iteration_warning: int,
      start_timestamp: pd.Timestamp,
+      relative_convergence_threshold: float | None = 1e-6,
+      relative_convergence_streak: int = 20,


@gemini-code-assist how are these new attributes used in this PR?

These new attributes, relative_convergence_threshold and relative_convergence_streak, are introduced to enhance the simulator's convergence criteria. They enable an early stopping mechanism for the finite difference method (FDM) iterations.

Specifically:

relative_convergence_threshold: If set (not None), the simulator will consider itself converged and stop iterating if the absolute change in max_delta (which likely represents the maximum temperature change between iterations) falls below this threshold for a certain number of consecutive iterations.

relative_convergence_streak: This defines how many consecutive iterations the max_delta change must remain below the relative_convergence_threshold before the simulator stops early.

This allows for more efficient simulations by preventing unnecessary iterations once a stable state is reached, even if the absolute iteration_limit hasn't been met.

s2t2 · 2026-03-31T14:17:30Z

smart_control/simulator/weather_controller.py

+# 2. Circular-import risk: building_radiation_utils imports weather_controller
+#    (guarded by TYPE_CHECKING) to type-hint IrradianceComponents.  If pvlib
+#    were imported at module load time here, and pvlib itself (or a future
+#    transitive dependency) ever imported anything from smart_control, that


at this time let's assume pvlib won't be importing from smart_control

s2t2 · 2026-03-31T14:22:23Z

smart_control/simulator/weather_controller.py

+#    any user who has not installed the "solar" extras group, even if they never
+#    use the irradiance / solar-position features.
+#
+# 2. Circular-import risk: building_radiation_utils imports weather_controller


if we need to move the IrradianceComponents to avoid a circular import, let's consider moving it to its own file in the simulator directory, perhaps named "building_radiation_models.py". We can have this weather controller and the radiation utils import from there. I think that should avoid a circular import, right?

s2t2 · 2026-03-31T14:25:48Z

smart_control/simulator/weather_controller.py

+
+  def get_current_irradiance(
+      self, timestamp: pd.Timestamp
+  ) -> 'IrradianceComponents':


Let's use the real class name here as a type hint

don't understand what it means.

s2t2 · 2026-03-31T14:34:36Z

smart_control/simulator/weather_controller_test.py

+    )
+
+
+class WeatherControllerIrradianceTest(IrradianceTestBase):


Nice inheritance of the test classes.

Since these three Irradiance test classes have so many lines of code, let's move them to their own test files, like "weather_controller_irradiance_test.py" or something like that. This will make them easier to review and maintain.

ecosang · 2026-03-31T18:25:37Z

I am kind of lost and can't figure it out. Let's clarify where to put which codes and what to create for a new testing file. Then, I will make a new PR, and I will drop this one.

Put weather_test file to where?
Make the sbsim working without pvlib?
pvlib optional dependency?
create a new class for the irrdiance test.
Split relative_threshold in another PR.
make irrdiance data and station as new data class.

Maybe we can pass an instance of the WeatherStation class I suggested, and use that here.

This would allow us to remove the following params from the initialization:

station_json_path: str | None = None,
timezone: str | None = None,
latitude: float | None = None,
longitude: float | None = None,
In favor of:

self.latitude = self.station.lat
self.longitude = self.station.lon
self.time_zone = self.station.time_zone
s2t2 replied 

s2t2
Collaborator
If the station_info is now a required parameter for the weather controller, we can pass it as none, and create one using default values if one is not passed:

# Args:

station: WeatherStation | None = None

# Init
self.station = station or WeatherStation() # use default values for building SB1

remove _ensure_timestamp_tz


def _validate_timestamp(self, timestamp):
    timestamp = self._ensure_timestamp_tz(timestamp)
    min_time = min(self._weather_data['Time'])
    if timestamp < min_time:
      raise ValueError(
          f'Attempting to get weather data at {timestamp}, before the earliest'
          f' timestamp {min_time}.'
      )
    max_time = max(self._weather_data['Time'])
    if timestamp > max_time:
      raise ValueError(
          f'Attempting to get weather data at {timestamp}, after the latest'
          f' timestamp {max_time}.'
      )
   return timestamp

remove self.weather_data
define constatns at the top


_OUTSIDE_AIR_TEMP_SENSOR: Final[str] = 'outside_air_temperature_sensor'
_DEW_POINT_TEMP_SENSOR: Final[str] = 'dew_point_temperature_sensor'
_CLOUD_COVER_SENSOR: Final[str] = 'cloud_cover_sensor'
_GHI_SENSOR: Final[str] = 'ghi_sensor'
_DNI_SENSOR: Final[str] = 'dni_sensor'
_DHI_SENSOR: Final[str] = 'dhi_sensor'
Let's consider making all the specific important column names into constants, like SKY_TEMP_COL="'TempSkyC'" or SKY_TEMPERATURE_COLUMN_NAME="TempSkyC"

Put irradiance stuff in weather_controller by default? or all optional? then how to deal with pvlib?

Anything I missed?

add solar calculation in weather_controller and define fenestration c…

00b8c5e

…onstants, and add relative convergence argument in simulator

add weather test file

16bb1b8

s2t2 requested changes Mar 25, 2026

View reviewed changes

ecosang added 2 commits March 29, 2026 23:00

add testing cases, and clean up some constants for weather_controller…

265540f

…, also move testing csv files into raditaion util folder

add pvlib in optional depedency.

3bf35f5

s2t2 requested changes Mar 31, 2026

View reviewed changes



		def calculate_poa_irradiance(
		irradiance_components: 'weather_controller.IrradianceComponents',

Conversation

ecosang commented Mar 16, 2026

Uh oh!

ecosang commented Mar 16, 2026

Uh oh!

s2t2 commented Mar 19, 2026

Uh oh!

ecosang commented Mar 19, 2026

Uh oh!

s2t2 commented Mar 23, 2026

Uh oh!

s2t2 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

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!

s2t2 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!

gemini-code-assist bot Mar 31, 2026

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!

ecosang commented Mar 31, 2026 •

edited

Loading