Mrt/issue 246 moving in from forked #262
Conversation
WalkthroughAdds sky-emissivity calculation modules and result types, extends input dataclasses with tdp/eps_sky/fcn, introduces an MRT calculation registry, adds tests for emissivity implementations, and includes placeholder modules for ground and sky temperature helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SkyEmissivity
participant Validator
participant Units
participant SkyEmissivityResult
participant Eps_Sky
User->>SkyEmissivity: brunt(tdp, units)
activate SkyEmissivity
SkyEmissivity->>Validator: validate inputs (SkyEmissivityBruntInputs)
SkyEmissivity->>Units: convert if IP -> SI
SkyEmissivity->>SkyEmissivity: compute eps = 0.741 + 0.0062*tdp
SkyEmissivity->>SkyEmissivity: clip eps to [0,1]
SkyEmissivity->>SkyEmissivityResult: return result(eps_sky)
deactivate SkyEmissivity
User->>SkyEmissivityResult: apply_dilley()
activate SkyEmissivityResult
SkyEmissivityResult->>SkyEmissivityResult: apply multiplier, clamp to 1.0
SkyEmissivityResult->>Eps_Sky: create and return
deactivate SkyEmissivityResult
sequenceDiagram
participant User
participant MRT
participant Registry
participant MethodImpl
User->>MRT: calculate(method=ForthPower, **kwargs)
activate MRT
MRT->>Registry: lookup ForthPower
Registry-->>MRT: method impl (mrt_simple)
MRT->>MethodImpl: invoke mrt_simple(**kwargs)
MethodImpl-->>MRT: float result
MRT->>User: return result
deactivate MRT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pythermalcomfort/classes_input.py (1)
203-204: Duplicate docstring line.There are two identical docstring lines back‑to‑back; remove one.
- """Validate and normalize fields using metadata declared on each field.""" - """Validate and normalize fields using metadata declared on each field.""" + """Validate and normalise fields using metadata declared on each field."""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
pythermalcomfort/classes_input.py(4 hunks)pythermalcomfort/classes_return.py(1 hunks)pythermalcomfort/environment/ground_temperature.py(1 hunks)pythermalcomfort/environment/mrt.py(1 hunks)pythermalcomfort/environment/sky_emissivity.py(1 hunks)pythermalcomfort/environment/sky_emissivity_2.py(1 hunks)pythermalcomfort/environment/sky_temperature.py(1 hunks)tests/test_sky_emissivity.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
tests/**/*.py: Write tests covering scalars, arrays, broadcasting, and invalid inputs (TypeError/ValueError)
Use numpy.testing or pytest.approx for numeric assertions
Ensure tests are deterministic, small, and pass locally with pytest before PR
Files:
tests/test_sky_emissivity.py
pythermalcomfort/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
pythermalcomfort/**/*.py: Use type hints and NumPy-style docstrings for all library code
Validate inputs in dataclass post_init (call super().post_init() if inheriting BaseInputs)
Convert pandas Series to list/np.array and normalize numeric inputs with np.asarray in post_init
Use validate_type(...) for type checks; raise TypeError for wrong types and ValueError for invalid values
Enforce physical constraints in validation (e.g., non-negativity, z2>z0, z1>z0, z2>z1) with clear error messages
Use numpy operations for numerical work (e.g., np.log, np.asarray) and avoid math.log when arrays are accepted
Validate domains before logs/roots (arguments must be > 0)
Support vectorized behavior; use np.atleast_1d, np.broadcast_to, and check broadcast compatibility
Follow repository patterns: dataclasses, BaseInputs metadata, validate_type, Units/Enums (Postures, WorkIntensity, Sex)
Functions should be small, single-purpose, and include concise, runnable examples in NumPy-style docstrings
Run ruff (lint/format) and docformatter; ensure code is formatted before PR
When accepting arrays, ensure computations are vectorized and avoid per-element Python loops
Files:
pythermalcomfort/environment/ground_temperature.pypythermalcomfort/environment/sky_temperature.pypythermalcomfort/classes_return.pypythermalcomfort/environment/mrt.pypythermalcomfort/environment/sky_emissivity.pypythermalcomfort/classes_input.pypythermalcomfort/environment/sky_emissivity_2.py
pythermalcomfort/classes_input.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Add or update input dataclasses in pythermalcomfort/classes_input.py (or adjacent to the function when appropriate)
Files:
pythermalcomfort/classes_input.py
🧠 Learnings (2)
📚 Learning: 2025-09-15T01:23:02.859Z
Learnt from: CR
PR: CenterForTheBuiltEnvironment/pythermalcomfort#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-15T01:23:02.859Z
Learning: Applies to pythermalcomfort/classes_input.py : Add or update input dataclasses in pythermalcomfort/classes_input.py (or adjacent to the function when appropriate)
Applied to files:
pythermalcomfort/classes_input.py
📚 Learning: 2025-09-15T01:23:02.859Z
Learnt from: CR
PR: CenterForTheBuiltEnvironment/pythermalcomfort#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-15T01:23:02.859Z
Learning: Applies to pythermalcomfort/**/*.py : Validate inputs in dataclass __post_init__ (call super().__post_init__() if inheriting BaseInputs)
Applied to files:
pythermalcomfort/classes_input.py
🧬 Code graph analysis (4)
tests/test_sky_emissivity.py (3)
pythermalcomfort/environment/sky_emissivity.py (4)
SkyEmissivity(38-163)SkyEmissivityResult(22-35)brunt(42-72)apply_dilley(30-33)pythermalcomfort/environment/sky_emissivity_2.py (6)
SkyEmissivity(51-118)EpsSky(21-47)brunt(55-72)dilly(26-28)prata(31-33)example_correction_with_inputs(36-47)pythermalcomfort/classes_return.py (1)
Eps_Sky(366-376)
pythermalcomfort/environment/sky_emissivity.py (4)
pythermalcomfort/classes_input.py (3)
SkyEmissivityBruntInputs(734-741)SkyEmissivitySwinbankInputs(745-752)SkyEmissivityClarkAllenInputs(755-764)pythermalcomfort/utilities.py (2)
Units(36-40)units_converter(786-824)pythermalcomfort/classes_return.py (2)
AutoStrMixin(10-37)Eps_Sky(366-376)pythermalcomfort/environment/sky_emissivity_2.py (5)
EpsSky(21-47)SkyEmissivity(51-118)brunt(55-72)swinbank(75-93)clark_allen(96-118)
pythermalcomfort/classes_input.py (1)
pythermalcomfort/models/jos3.py (2)
tdb(1403-1414)tdb(1417-1418)
pythermalcomfort/environment/sky_emissivity_2.py (3)
pythermalcomfort/classes_input.py (3)
SkyEmissivityBruntInputs(734-741)SkyEmissivitySwinbankInputs(745-752)SkyEmissivityClarkAllenInputs(755-764)pythermalcomfort/utilities.py (2)
Units(36-40)units_converter(786-824)pythermalcomfort/environment/sky_emissivity.py (4)
SkyEmissivity(38-163)brunt(42-72)swinbank(76-117)clark_allen(119-163)
🔇 Additional comments (1)
pythermalcomfort/classes_return.py (1)
365-377: Let me search the codebase directly for usage of both classes:Verify return type consolidation before proceeding
The identified issue is valid: both
EpsSky(inenvironment/sky_emissivity_2.py) andEps_Sky(inclasses_return.py) represent the same concept with inconsistent naming and type signatures. Consolidation would improve maintainability and reduce user confusion. However, the consolidation impact depends on:
- Which class is actively used in public-facing functions—check return type annotations in models/utilities that reference sky emissivity
- Whether
environment/sky_emissivity_2.pyis part of the public API or legacy code- Type hint differences (
Union[float, np.ndarray]vsfloat | list[float])—align these before consolidatingRecommend auditing callers of both classes and assessing whether
AutoStrMixin(currently only inEps_Sky) should be applied to the unified type. Once confirmed, rename and update all return type annotations across the codebase.
| """ | ||
| Base inputs with metadata-driven validation. | ||
| Parameters | ||
| ---------- | ||
| tdb : float | ||
| Dry bulb temperature in degrees Celsius (°C). | ||
| eps_sky : float | ||
| Sky emissivity, physically constrained between 0 and 1. | ||
| tdp : float | ||
| Dew point temperature in degrees Celsius (°C). | ||
| fcn : float | ||
| Fraction | ||
| """ |
There was a problem hiding this comment.
BaseInputs docstring is partial; either document comprehensively or move specific notes.
The docstring lists only tdb, eps_sky, tdp, fcn while BaseInputs exposes many fields. Clarify scope or relocate sky‑emissivity specifics to the dedicated input classes to avoid confusion.
🤖 Prompt for AI Agents
In pythermalcomfort/classes_input.py around lines 24 to 37, the BaseInputs
docstring only documents tdb, eps_sky, tdp and fcn while the class exposes many
more fields; update the docstring so it is accurate by either (A) expanding the
Parameters section to list and describe every public attribute (name, type,
units/constraints) present on BaseInputs, or (B) replace the detailed parameters
with a concise, top-level description of BaseInputs and remove the
sky‑emissivity specifics from this docstring, then add the eps_sky constraint
note to the dedicated sky/emissivity input class docstring instead; ensure
parameter names, types and any physical constraints match the actual class
attributes and keep wording consistent with project style.
pythermalcomfort/classes_input.py
Outdated
| eps_sky: float | int | np.ndarray |list = field( | ||
| default=0.7, metadata={"types": (float, int, np.ndarray, list)} | ||
| ) | ||
| fcn: float | int | np.ndarray |list = field( | ||
| default=1, metadata={"types": (float, int, np.ndarray, list)} | ||
| ) | ||
|
|
There was a problem hiding this comment.
Don’t bake outputs or defaults into BaseInputs; set eps_sky and fcn to None.
eps_sky is an output, and fcn (cloud fraction) defaulting to 1 can surprise callers. Make them optional (None) and set explicit values in dedicated input classes.
- eps_sky: float | int | np.ndarray |list = field(
- default=0.7, metadata={"types": (float, int, np.ndarray, list)}
- )
- fcn: float | int | np.ndarray |list = field(
- default=1, metadata={"types": (float, int, np.ndarray, list)}
- )
+ eps_sky: float | int | np.ndarray | list = field(
+ default=None, metadata={"types": (float, int, np.ndarray, list)}
+ )
+ fcn: float | int | np.ndarray | list = field(
+ default=None, metadata={"types": (float, int, np.ndarray, list)}
+ )Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pythermalcomfort/classes_input.py around lines 195 to 201, change eps_sky and
fcn so they are optional with default None instead of defaulting to 0.7 and 1;
update their type annotations to include None (e.g. Optional[float | int |
np.ndarray | list]) and set field(default=None, metadata={"types": (float, int,
np.ndarray, list, type(None))}) so runtime type checks accept None; then remove
baked defaults from BaseInputs and ensure any derived or dedicated input classes
set explicit eps_sky and fcn values where needed.
pythermalcomfort/classes_input.py
Outdated
| @dataclass | ||
| class SkyEmissivityClarkAllenInputs(BaseInputs): | ||
| def __init__( | ||
| self, | ||
| tdb, | ||
| fcn | ||
| ): | ||
| super().__init__( | ||
| tdb=tdb, | ||
| fcn = fcn | ||
| ) |
There was a problem hiding this comment.
Clark–Allen inputs: parameter should be dew point (tdp), and validate fcn ∈ [0,1].
The model uses dew point, but this class uses tdb. Also add a post_init to enforce cloud fraction bounds.
-@dataclass
-class SkyEmissivityClarkAllenInputs(BaseInputs):
- def __init__(
- self,
- tdb,
- fcn
- ):
- super().__init__(
- tdb=tdb,
- fcn = fcn
- )
+@dataclass
+class SkyEmissivityClarkAllenInputs(BaseInputs):
+ """Inputs for the Clark & Allen (1978) sky emissivity model.
+
+ Parameters
+ ----------
+ tdp : float | int | ndarray | list
+ Dew point temperature in °C.
+ fcn : float | int | ndarray | list
+ Cloud fraction in [0, 1].
+ """
+ def __init__(self, tdp, fcn):
+ super().__init__(tdp=tdp, fcn=fcn)
+
+ def __post_init__(self):
+ super().__post_init__()
+ fcn = np.asarray(self.fcn, dtype=float)
+ if np.any((fcn < 0) | (fcn > 1)):
+ raise ValueError("fcn (cloud fraction) must be within [0, 1].")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pythermalcomfort/classes_input.py around lines 754 to 764, the
SkyEmissivityClarkAllenInputs constructor uses tdb but the Clark–Allen model
expects dew point (tdp) and it does not validate cloud fraction bounds; rename
the constructor parameter from tdb to tdp and pass tdp into super().__init__
(ensuring the kwarg name matches the BaseInputs API), and add a __post_init__
that validates fcn is between 0 and 1 (raise ValueError if outside) or clamps it
as your codebase prefers, with a clear error message referencing the class and
field.
| """ | ||
| This module handles helper methods to estimate ground temperatures | ||
| """ | ||
|
|
||
| # Same structure as sky_emissivity, just no correction methods. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same here — drop or stub with a TODO
No functional code. Prefer excluding until implemented, or add a TODO with a linked issue so it doesn’t ship as a no-op module.
🤖 Prompt for AI Agents
pythermalcomfort/environment/ground_temperature.py lines 1-5: this module
currently contains only a docstring and no functional code — either remove the
file or replace it with a minimal stub that clearly indicates it’s
unimplemented; add a top-level TODO comment referencing a tracking issue (e.g.,
TODO: implement ground temperature helpers — issue #1234) and implement
placeholder behavior such as raising NotImplementedError from any exported
function or leaving an explicit stub function with a descriptive docstring so
the package does not ship a silent no-op module.
| def test_sky_emissivity_chain() -> None: | ||
| chained: SkyEmissivityResult = SkyEmissivity.brunt(tdp=10.0).apply_dilley() | ||
| assert isinstance(chained, Eps_Sky) | ||
| assert 0.0 <= chained.eps_sky <= 1.0 | ||
|
|
There was a problem hiding this comment.
Type annotation: chain result
The chained call yields Eps_Sky.
- chained: SkyEmissivityResult = SkyEmissivity.brunt(tdp=10.0).apply_dilley()
+ chained: Eps_Sky = SkyEmissivity.brunt(tdp=10.0).apply_dilley()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_sky_emissivity_chain() -> None: | |
| chained: SkyEmissivityResult = SkyEmissivity.brunt(tdp=10.0).apply_dilley() | |
| assert isinstance(chained, Eps_Sky) | |
| assert 0.0 <= chained.eps_sky <= 1.0 | |
| def test_sky_emissivity_chain() -> None: | |
| chained: Eps_Sky = SkyEmissivity.brunt(tdp=10.0).apply_dilley() | |
| assert isinstance(chained, Eps_Sky) | |
| assert 0.0 <= chained.eps_sky <= 1.0 |
🤖 Prompt for AI Agents
In tests/test_sky_emissivity.py around lines 30 to 34, the local variable
`chained` is annotated as SkyEmissivityResult but the chained call returns an
Eps_Sky; update the type annotation to Eps_Sky and ensure Eps_Sky is imported at
the top of the file so the test type matches the actual return value.
| def test_sky_emissivity_chain_array() -> None: | ||
| tdp_array = [0.0, 5.0, 10.0, 20.0] | ||
| base: SkyEmissivityResult = SkyEmissivity.brunt(tdp=tdp_array) | ||
| corrected: SkyEmissivityResult = base.apply_dilley() | ||
| assert isinstance(corrected.eps_sky, np.ndarray) | ||
| assert np.all((0.0 <= corrected.eps_sky) & (corrected.eps_sky <= 1.0)) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type of corrected and array-safe assertions
apply_dilley() returns Eps_Sky. Keep array-safe expectations and checks.
- base: SkyEmissivityResult = SkyEmissivity.brunt(tdp=tdp_array)
- corrected: SkyEmissivityResult = base.apply_dilley()
+ base: SkyEmissivityResult = SkyEmissivity.brunt(tdp=tdp_array)
+ corrected: Eps_Sky = base.apply_dilley()🤖 Prompt for AI Agents
In tests/test_sky_emissivity.py around lines 35-41, the test assumes
apply_dilley() returns a SkyEmissivityResult but it actually returns an Eps_Sky
(or an object whose eps_sky is the ndarray); change the type annotation from
SkyEmissivityResult to Eps_Sky (or the correct return type), call apply_dilley()
accordingly, and keep array-safe assertions: assert that corrected.eps_sky is an
np.ndarray and that all its values are within [0,1] (use np.all with a boolean
mask).
| def test_sky_emissivity2_dilley() -> None: | ||
| base: Eps_Sky_2 = SkyEmissivity_2.brunt(tdp=10.0) | ||
| corrected = Eps_Sky_2(eps_sky=Eps_Sky_2.dilly(base.eps_sky)) | ||
| expected = min(1.0, base.eps_sky * 1.05) | ||
| assert np.isclose(corrected.eps_sky, expected, atol=1e-6) | ||
|
|
There was a problem hiding this comment.
Use np.minimum for corrections (v2)
Ensure the expected value is array-safe.
- expected = min(1.0, base.eps_sky * 1.05)
+ expected = np.minimum(1.0, base.eps_sky * 1.05)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_sky_emissivity2_dilley() -> None: | |
| base: Eps_Sky_2 = SkyEmissivity_2.brunt(tdp=10.0) | |
| corrected = Eps_Sky_2(eps_sky=Eps_Sky_2.dilly(base.eps_sky)) | |
| expected = min(1.0, base.eps_sky * 1.05) | |
| assert np.isclose(corrected.eps_sky, expected, atol=1e-6) | |
| def test_sky_emissivity2_dilley() -> None: | |
| base: Eps_Sky_2 = SkyEmissivity_2.brunt(tdp=10.0) | |
| corrected = Eps_Sky_2(eps_sky=Eps_Sky_2.dilly(base.eps_sky)) | |
| expected = np.minimum(1.0, base.eps_sky * 1.05) | |
| assert np.isclose(corrected.eps_sky, expected, atol=1e-6) |
🤖 Prompt for AI Agents
In tests/test_sky_emissivity.py around lines 57 to 62, the test computes
expected using Python's built-in min which is not array-safe; replace that with
numpy's nan-aware vectorized operation (use np.minimum(1.0, base.eps_sky *
1.05)) so the test works with scalar or array eps_sky values and remains robust
in vectorized contexts.
| def test_sky_emissivity2_prata() -> None: | ||
| base: Eps_Sky_2 = SkyEmissivity_2.brunt(tdp=10.0) | ||
| corrected = Eps_Sky_2(eps_sky=Eps_Sky_2.prata(base.eps_sky)) | ||
| expected = min(1.0, base.eps_sky * 1.03) | ||
| assert np.isclose(corrected.eps_sky, expected, atol=1e-6) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same here for Prata
Switch to np.minimum.
- expected = min(1.0, base.eps_sky * 1.03)
+ expected = np.minimum(1.0, base.eps_sky * 1.03)🤖 Prompt for AI Agents
In tests/test_sky_emissivity.py around lines 63 to 68, the test uses Python
built-in min() to compute the expected emissivity; replace that with numpy's
np.minimum to ensure array-compatible and consistent numeric behavior (e.g.,
expected = np.minimum(1.0, base.eps_sky * 1.03)), then run tests to confirm no
regressions.
tests/test_sky_emissivity.py
Outdated
| def test_sky_emissivity2_example_correction_with_inputs() -> None: | ||
| base: Eps_Sky_2 = SkyEmissivity_2.brunt(tdp=10.0) | ||
|
|
||
| # Apply example correction (currently just multiplies by 1.03, clipped at 1) | ||
| corrected = Eps_Sky_2( | ||
| eps_sky=Eps_Sky_2.example_correction_with_inputs(base.eps_sky) | ||
| ) | ||
| expected = min(1.0, base.eps_sky * 1.03) | ||
| assert np.isclose(corrected.eps_sky, expected, atol=1e-6) No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify example correction expectation
Use np.minimum to be robust to array inputs.
- expected = min(1.0, base.eps_sky * 1.03)
+ expected = np.minimum(1.0, base.eps_sky * 1.03)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_sky_emissivity2_example_correction_with_inputs() -> None: | |
| base: Eps_Sky_2 = SkyEmissivity_2.brunt(tdp=10.0) | |
| # Apply example correction (currently just multiplies by 1.03, clipped at 1) | |
| corrected = Eps_Sky_2( | |
| eps_sky=Eps_Sky_2.example_correction_with_inputs(base.eps_sky) | |
| ) | |
| expected = min(1.0, base.eps_sky * 1.03) | |
| assert np.isclose(corrected.eps_sky, expected, atol=1e-6) | |
| def test_sky_emissivity2_example_correction_with_inputs() -> None: | |
| base: Eps_Sky_2 = SkyEmissivity_2.brunt(tdp=10.0) | |
| # Apply example correction (currently just multiplies by 1.03, clipped at 1) | |
| corrected = Eps_Sky_2( | |
| eps_sky=Eps_Sky_2.example_correction_with_inputs(base.eps_sky) | |
| ) | |
| expected = np.minimum(1.0, base.eps_sky * 1.03) | |
| assert np.isclose(corrected.eps_sky, expected, atol=1e-6) |
🤖 Prompt for AI Agents
In tests/test_sky_emissivity.py around lines 76 to 84, the test computes
expected using Python's built-in min which fails for numpy arrays; replace
expected = min(1.0, base.eps_sky * 1.03) with expected = np.minimum(1.0,
base.eps_sky * 1.03) so the clipping works elementwise and the assertion is
robust to array inputs.
| This module handles the main Mean Radiant Temperature MRT functions inlcuding helper methods to calucalte longwave radiation | ||
| """ | ||
|
|
||
| # copy over 4th power formular called: forth_power, this mrt.forth_power(...) |
There was a problem hiding this comment.
remove commented out code, since we do not want it in production
|
@KristinaM93 I did not see your comments to this pull request. Have you posted them? |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (23)
pythermalcomfort/environment/mrt.py (6)
12-16: Enum names: fix typos and remove placeholder [duplicate]Correct spelling and remove dead member.
class MRTMethod(Enum): - ForthPower = "4Power" + FourthPower = "4Power" NoGeometry = "NoGeometry" - AnotherDifferentMehtod = "AnotherDifferentMehtod"
23-24: Duplicate method registry; keep one dict keyed by MRTMethod [duplicate]Two _methods attributes with incompatible key types; collapse into one.
- _methods: dict[str, Callable[..., float]] = {} @@ - _methods: dict[MRTMethod, Callable[..., float]] = { - MRTMethod.ForthPower: mrt_simple.__func__, + _methods: dict[MRTMethod, Callable[..., float]] = { + MRTMethod.FourthPower: mrt_simple.__func__, MRTMethod.NoGeometry: mrt_asw.__func__, }Also applies to: 34-37
26-33: Vectorise helpers; drop unused param; add NumPy‑style docs [duplicate]Support scalars/arrays; remove unused v or accept **kwargs.
+import numpy as np @@ - def mrt_simple(tdb: float, tr: float, v: float) -> float: - return 0.5 * (tdb + tr) + def mrt_simple(tdb: float | np.ndarray, tr: float | np.ndarray, **kwargs) -> np.ndarray: + """Mean of air and radiant temps. + + Parameters + ---------- + tdb, tr : array_like + Dry‑bulb and radiant temperature [°C]. + """ + tdb_arr = np.asarray(tdb, dtype=float) + tr_arr = np.asarray(tr, dtype=float) + return 0.5 * (tdb_arr + tr_arr) @@ - def mrt_asw(asw: float, tdb: float, tr: float) -> float: - return asw * tdb + (1 - asw) * tr + def mrt_asw(asw: float | np.ndarray, tdb: float | np.ndarray, tr: float | np.ndarray) -> np.ndarray: + """Area‑weighted MRT.""" + asw_arr = np.asarray(asw, dtype=float) + tdb_arr = np.asarray(tdb, dtype=float) + tr_arr = np.asarray(tr, dtype=float) + return asw_arr * tdb_arr + (1.0 - asw_arr) * tr_arr
39-43: calculate(): docstring, typing, clearer errorPolish signature and message.
- def calculate(cls, method: MRTMethod, **kwargs) -> float: - if method not in cls._methods: - raise ValueError(f"Unknown MRT method '{method}'") + def calculate(cls, method: MRTMethod, **kwargs): + """Dispatch to the selected MRT method.""" + if method not in cls._methods: + raise ValueError(f"Unknown MRT method: {method!s}") return cls._methods[method](**kwargs)
46-52: Don’t execute on import; guard examples [duplicate]Move example calls under main guard.
-# ---------------------- -# Testing: -# ---------------------- -mrt_val = MRT.calculate(MRTMethod.ForthPower, tdb=25, tr=30, v=0.1) -mrt_val2 = MRT.calculate(MRTMethod.NoGeometry, tdb=25, tr=30, asw=0.6) - -print(mrt_val, mrt_val2) +if __name__ == "__main__": + mrt_val = MRT.calculate(MRTMethod.FourthPower, tdb=25, tr=30) + mrt_val2 = MRT.calculate(MRTMethod.NoGeometry, tdb=25, tr=30, asw=0.6) + print(mrt_val, mrt_val2)
4-6: Docstring typos and stray commentsFix spelling; drop placeholder comments per earlier feedback.
-""" -This module handles the main Mean Radiant Temperature MRT functions inlcuding helper methods to calucalte longwave radiation -""" +"""Mean Radiant Temperature (MRT) helpers, including longwave radiation utilities.""" @@ -# copy over 4th power formular called: forth_power, this mrt.forth_power(...) -# add +# (removed placeholder comments)Also applies to: 8-11
pythermalcomfort/classes_input.py (2)
24-37: BaseInputs docstring: clarify scope or move specifics [duplicate]Docstring lists tdb/eps_sky/tdp/fcn only, but BaseInputs exposes many fields. Either document comprehensively or move sky‑emissivity specifics to dedicated input classes.
- """ - Base inputs with metadata-driven validation. - ... - tdb : float - Dry bulb temperature in degrees Celsius (°C). - eps_sky : float - Sky emissivity, physically constrained between 0 and 1. - tdp : float - Dew point temperature in degrees Celsius (°C). - fcn : float - Fraction - """ + """ + Base inputs with metadata‑driven validation. + + Notes + ----- + This class is generic; parameter‑specific details (e.g., sky emissivity inputs) + should be documented on specialised input subclasses. + """
195-200: Don’t bake outputs/defaults into BaseInputs; make eps_sky/fcn optional [duplicate]These should be None by default; enforce ranges where they’re actually used.
- eps_sky: float | int | np.ndarray | list = field( - default=0.7, metadata={"types": (float, int, np.ndarray, list)} - ) - fcn: float | int | np.ndarray | list = field( - default=1, metadata={"types": (float, int, np.ndarray, list)} - ) + eps_sky: float | int | np.ndarray | list = field( + default=None, metadata={"types": (float, int, np.ndarray, list)} + ) + fcn: float | int | np.ndarray | list = field( + default=None, metadata={"types": (float, int, np.ndarray, list)} + )pythermalcomfort/classes_return.py (1)
366-377: Eps_Sky: support arrays and enforce [0,1] [duplicate]Add array‑friendly typing and clamp/validate emissivity range.
@dataclass(frozen=True, repr=False) class Eps_Sky(AutoStrMixin): @@ - eps_sky: float | list[float] + eps_sky: npt.ArrayLike + + def __post_init__(self) -> None: + arr = np.asarray(self.eps_sky, dtype=float) + if np.any((arr < 0.0) | (arr > 1.0)): + raise ValueError("eps_sky must be within [0, 1].")tests/test_sky_emissivity.py (4)
31-35: Use Eps_Sky return type and array‑safe expectation [duplicate]
apply_dilley()returns Eps_Sky; use np.minimum for clipping.- base: SkyEmissivityResult = SkyEmissivity.brunt(tdp=10.0) - corrected: SkyEmissivityResult = base.apply_dilley() - expected = min(1.0, base.eps_sky * 1.05) + base: SkyEmissivityResult = SkyEmissivity.brunt(tdp=10.0) + corrected: Eps_Sky = base.apply_dilley() + expected = np.minimum(1.0, base.eps_sky * 1.05)
38-41: Chained return is Eps_Sky [duplicate]Fix annotation.
- chained: SkyEmissivityResult = SkyEmissivity.brunt(tdp=10.0).apply_dilley() + chained: Eps_Sky = SkyEmissivity.brunt(tdp=10.0).apply_dilley()
44-49: Type of corrected is Eps_Sky [duplicate]Align with API.
- corrected: SkyEmissivityResult = base.apply_dilley() + corrected: Eps_Sky = base.apply_dilley()
69-74: Expected values: use np.minimum for array‑safe clipping [duplicate]Replace Python min(...) with np.minimum(...).
- expected = min(1.0, base.eps_sky * 1.05) + expected = np.minimum(1.0, base.eps_sky * 1.05) @@ - expected = min(1.0, base.eps_sky * 1.03) + expected = np.minimum(1.0, base.eps_sky * 1.03) @@ - expected = min(1.0, base.eps_sky * 1.03) + expected = np.minimum(1.0, base.eps_sky * 1.03)Also applies to: 76-81, 91-99
pythermalcomfort/environment/sky_emissivity_2.py (5)
15-20: Standardise on ‘Dilley’; fix example and method name [duplicate]Align naming and API in comments and code.
-# eps_sky = SkyEmissivity.brunt(tdp=10) -# eps_sky = SkyEmissivity.brunt(tdp=10, correction=EpsSky.apply_dilley) +# eps_sky = SkyEmissivity.brunt(tdp=10) +# eps_sky = SkyEmissivity.brunt(tdp=10, correction=EpsSky.dilley) @@ - def dilly(eps: float | np.ndarray) -> float | np.ndarray: - """Dilley correction (max 1.0).""" + def dilley(eps: float | np.ndarray) -> float | np.ndarray: + """Dilley correction (max 1.0).""" return np.minimum(1.0, np.asarray(eps) * 1.05)Also applies to: 28-36
22-27: Docstrings and typing: NumPy‑style docs, Literal units, flexible corrections [duplicate]Use Literal for units and accept corrections with kwargs.
-from dataclasses import dataclass +from dataclasses import dataclass +from typing import Optional +from typing_extensions import Literal @@ class EpsSky: - """Immutable container for sky emissivity value.""" + """Immutable container for sky emissivity value. + + Attributes + ---------- + eps_sky : float or ndarray + Sky emissivity in [0, 1]. + """ @@ - def brunt( - tdp: float | list[float], - units: str = Units.SI.value, - correction: Callable[[float | np.ndarray], float | np.ndarray] | None = None, + def brunt( + tdp: float | list[float], + units: Literal["SI", "IP"] = Units.SI.value, + correction: Optional[Callable[..., float | np.ndarray]] = None, ) -> EpsSky: @@ - def swinbank( - tdb: float | list[float], - units: str = Units.SI.value, - correction: Callable[[float | np.ndarray], float | np.ndarray] | None = None, + def swinbank( + tdb: float | list[float], + units: Literal["SI", "IP"] = Units.SI.value, + correction: Optional[Callable[..., float | np.ndarray]] = None, ) -> EpsSky: @@ - def clark_allen( - tdp: float | list[float], - cloud_fraction: float | list[float] = 0.0, - units: str = Units.SI.value, - correction: Callable[[float | np.ndarray], float | np.ndarray] | None = None, + def clark_allen( + tdp: float | list[float], + cloud_fraction: float | list[float] = 0.0, + units: Literal["SI", "IP"] = Units.SI.value, + correction: Optional[Callable[..., float | np.ndarray]] = None, ) -> EpsSky:Also applies to: 55-61, 75-81, 96-104
64-67: Critical: IP conversion for dew point uses unsupported ‘tdp’ key [duplicate]utilities.units_converter doesn’t accept tdp → IndexError on [0]. Use a temporary workaround and patch utilities separately.
- if units.upper() == Units.IP.value: - tdp_arr = units_converter(from_units=Units.IP.value, tdp=tdp_arr)[0] + if units.upper() == Units.IP.value: + # Workaround until utilities.units_converter supports 'tdp' + tdp_arr = units_converter(from_units=Units.IP.value, tdb=tdp_arr)[0]Follow‑up: add ‘tdp’ handling in utilities.units_converter in both directions. Based on learnings.
Also applies to: 109-111
70-73: Enforce physical bounds after user corrections [duplicate]Re‑clip eps post‑correction.
- if correction: - eps = correction(eps) + if correction: + eps = np.clip(correction(eps), 0.0, 1.0)Apply similarly in swinbank and clark_allen (shown).
Also applies to: 91-94, 116-119
106-114: Validate domains: cloud fraction in [0,1], log argument > 0 [duplicate]Add guardrails before np.log.
- tdp_arr = np.array(tdp, dtype=float) - cloud_arr = np.array(cloud_fraction, dtype=float) + tdp_arr = np.asarray(tdp, dtype=float) + cloud_arr = np.asarray(cloud_fraction, dtype=float) + if np.any((cloud_arr < 0.0) | (cloud_arr > 1.0)): + raise ValueError("cloud_fraction must be within [0, 1].") @@ - T_k = tdp_arr + 273.15 + T_k = tdp_arr + 273.15 + if np.any(T_k <= 0.0): + raise ValueError("tdp must be > -273.15 °C to keep log(T_k) valid.") eps_clear = 0.787 + 0.764 * np.log(T_k)pythermalcomfort/environment/sky_emissivity.py (5)
37-40: Fix return type annotation — use Eps_Sky.The method signature declares
EpsSkybut that type isn't defined or imported. The actual return isEps_Sky(line 40). Update the annotation to match.- def apply_dilley(self) -> EpsSky: + def apply_dilley(self) -> Eps_Sky:
66-68: Update docstring return type to SkyEmissivityResult.The docstring declares
EpsSkyas the return type, but the method returnsSkyEmissivityResult. Fix the docstring to match the implementation.- EpsSky + SkyEmissivityResult Instance containing the computed sky emissivity (eps_sky), clipped to the physical range [0.0, 1.0].
72-73: units_converter doesn't support 'tdp' keyword — IP conversion will silently fail.Examining
utilities.units_converter, it only converts keys matching"tmp" in key or key == "tr" or key == "tdb"for temperature. The key"tdp"won't match, sounits_converterwill return an empty list and indexing[0]will raise anIndexError.Either patch
utilities.units_converterto handle"tdp"(map it to temperature conversion), or use a supported key like"tdb":if units.upper() == Units.IP.value: - tdp = units_converter(from_units=Units.IP.value, tdp=tdp)[0] + tdp = units_converter(from_units=Units.IP.value, tdb=tdp)[0]Based on learnings.
105-106: Update docstring return type to SkyEmissivityResult.The docstring declares
EpsSky, but the method returnsSkyEmissivityResult. Update for consistency.- EpsSky + SkyEmissivityResult A dataclass containing `eps_sky` (clipped to [0, 1]).
127-169: Critical: Missing @staticmethod, parameter name mismatches, and missing validations.This method has several blocking issues:
- Missing @staticmethod decorator — the method has no
selfparameter but also no@staticmethod, so calling it will fail.- Parameter name mismatches — signature uses
tdbandfcn, but the body references undefinedtdpandcloud_fraction(lines 154, 156, 157, 160), causingNameError.- units_converter won't handle 'tdp' — as with
brunt(), the converter doesn't support the'tdp'key (line 160).- Missing domain validation —
np.log(T_k)requiresT_k > 0(line 163).- Missing cloud_fraction bounds check — should validate
[0, 1].Apply this diff to fix all issues:
+ @staticmethod def clark_allen( - tdb: float | list[float], - fcn: float | list[float], - units: str = Units.SI.value, + tdp: float | list[float], + cloud_fraction: float | list[float] = 0.0, + units: Literal["SI", "IP"] = Units.SI.value, ) -> SkyEmissivityResult: """ Calculate sky emissivity using the Clark & Allen (1978) model. Parameters ---------- tdp : float or list of floats Dew point temperature in °C (SI) or °F (IP) cloud_fraction : float or list of floats, default=0.0 Fraction of cloud cover [0, 1] units : str, default "SI" Units system, "SI" = °C, "IP" = °F Returns ------- SkyEmissivityResult Object containing `eps_sky` with emissivity values (clipped to [0,1]) """ # Notes BG: Read here: https://doi.org/10.26868/25222708.2017.569 # https://www.proquest.com/openview/4763607fbd21956404a6329a060ae2b4/1?pq-origsite=gscholar&cbl=18750&diss=y SkyEmissivityClarkAllenInputs(tdp=tdp, fcn=cloud_fraction) tdp_arr = np.array(tdp, dtype=float) cloud_arr = np.array(cloud_fraction, dtype=float) + + if np.any((cloud_arr < 0) | (cloud_arr > 1)): + raise ValueError("cloud_fraction must be within [0, 1].") if units.upper() == Units.IP.value: - tdp_arr = units_converter(from_units=Units.IP.value, tdp=tdp_arr)[0] + tdp_arr = units_converter(from_units=Units.IP.value, tdb=tdp_arr)[0] T_k = tdp_arr + 273.15 + if np.any(T_k <= 0): + raise ValueError("tdp must be greater than -273.15 °C to keep log(T_k) valid.") epsilon_clear = 0.787 + 0.764 * np.log(T_k) Ca = 0.23 epsilon_sky = epsilon_clear * (1 + Ca * cloud_arr) epsilon_sky = np.clip(epsilon_sky, 0.0, 1.0) return SkyEmissivityResult(eps_sky=epsilon_sky)As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
pythermalcomfort/classes_input.py(4 hunks)pythermalcomfort/classes_return.py(1 hunks)pythermalcomfort/environment/mrt.py(1 hunks)pythermalcomfort/environment/sky_emissivity.py(1 hunks)pythermalcomfort/environment/sky_emissivity_2.py(1 hunks)tests/test_sky_emissivity.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
pythermalcomfort/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
pythermalcomfort/**/*.py: Use type hints and NumPy-style docstrings for all library code
Validate inputs in dataclass post_init (call super().post_init() if inheriting BaseInputs)
Convert pandas Series to list/np.array and normalize numeric inputs with np.asarray in post_init
Use validate_type(...) for type checks; raise TypeError for wrong types and ValueError for invalid values
Enforce physical constraints in validation (e.g., non-negativity, z2>z0, z1>z0, z2>z1) with clear error messages
Use numpy operations for numerical work (e.g., np.log, np.asarray) and avoid math.log when arrays are accepted
Validate domains before logs/roots (arguments must be > 0)
Support vectorized behavior; use np.atleast_1d, np.broadcast_to, and check broadcast compatibility
Follow repository patterns: dataclasses, BaseInputs metadata, validate_type, Units/Enums (Postures, WorkIntensity, Sex)
Functions should be small, single-purpose, and include concise, runnable examples in NumPy-style docstrings
Run ruff (lint/format) and docformatter; ensure code is formatted before PR
When accepting arrays, ensure computations are vectorized and avoid per-element Python loops
Files:
pythermalcomfort/classes_return.pypythermalcomfort/environment/sky_emissivity_2.pypythermalcomfort/environment/mrt.pypythermalcomfort/environment/sky_emissivity.pypythermalcomfort/classes_input.py
tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
tests/**/*.py: Write tests covering scalars, arrays, broadcasting, and invalid inputs (TypeError/ValueError)
Use numpy.testing or pytest.approx for numeric assertions
Ensure tests are deterministic, small, and pass locally with pytest before PR
Files:
tests/test_sky_emissivity.py
pythermalcomfort/classes_input.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Add or update input dataclasses in pythermalcomfort/classes_input.py (or adjacent to the function when appropriate)
Files:
pythermalcomfort/classes_input.py
🧠 Learnings (6)
📚 Learning: 2025-09-15T01:23:02.859Z
Learnt from: CR
PR: CenterForTheBuiltEnvironment/pythermalcomfort#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-15T01:23:02.859Z
Learning: Applies to pythermalcomfort/{models,utilities}.py : Ensure docstring sections include Args (with units), Returns, Raises, Examples, Applicability
Applied to files:
pythermalcomfort/environment/sky_emissivity_2.py
📚 Learning: 2025-09-15T01:23:02.859Z
Learnt from: CR
PR: CenterForTheBuiltEnvironment/pythermalcomfort#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-15T01:23:02.859Z
Learning: Applies to pythermalcomfort/models/*.py : Include applicability limits and minimal runnable examples in model function docstrings
Applied to files:
pythermalcomfort/environment/sky_emissivity_2.py
📚 Learning: 2025-09-15T01:23:02.859Z
Learnt from: CR
PR: CenterForTheBuiltEnvironment/pythermalcomfort#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-15T01:23:02.859Z
Learning: Applies to pythermalcomfort/**/*.py : Functions should be small, single-purpose, and include concise, runnable examples in NumPy-style docstrings
Applied to files:
pythermalcomfort/environment/sky_emissivity_2.py
📚 Learning: 2025-09-15T01:23:02.859Z
Learnt from: CR
PR: CenterForTheBuiltEnvironment/pythermalcomfort#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-15T01:23:02.859Z
Learning: Applies to pythermalcomfort/**/*.py : Follow repository patterns: dataclasses, BaseInputs metadata, validate_type, Units/Enums (Postures, WorkIntensity, Sex)
Applied to files:
pythermalcomfort/classes_input.py
📚 Learning: 2025-09-15T01:23:02.859Z
Learnt from: CR
PR: CenterForTheBuiltEnvironment/pythermalcomfort#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-15T01:23:02.859Z
Learning: Applies to pythermalcomfort/classes_input.py : Add or update input dataclasses in pythermalcomfort/classes_input.py (or adjacent to the function when appropriate)
Applied to files:
pythermalcomfort/classes_input.py
📚 Learning: 2025-09-15T01:23:02.859Z
Learnt from: CR
PR: CenterForTheBuiltEnvironment/pythermalcomfort#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-15T01:23:02.859Z
Learning: Applies to pythermalcomfort/**/*.py : Validate inputs in dataclass __post_init__ (call super().__post_init__() if inheriting BaseInputs)
Applied to files:
pythermalcomfort/classes_input.py
🧬 Code graph analysis (4)
tests/test_sky_emissivity.py (3)
pythermalcomfort/classes_return.py (1)
Eps_Sky(367-377)pythermalcomfort/environment/sky_emissivity.py (4)
SkyEmissivity(46-170)SkyEmissivityResult(28-42)brunt(50-80)apply_dilley(37-40)pythermalcomfort/environment/sky_emissivity_2.py (6)
SkyEmissivity(52-119)EpsSky(23-49)brunt(56-73)dilly(29-31)prata(34-36)example_correction_with_inputs(39-49)
pythermalcomfort/environment/sky_emissivity_2.py (2)
pythermalcomfort/classes_input.py (3)
SkyEmissivityBruntInputs(735-739)SkyEmissivityClarkAllenInputs(751-753)SkyEmissivitySwinbankInputs(743-747)pythermalcomfort/utilities.py (2)
Units(36-40)units_converter(786-824)
pythermalcomfort/environment/sky_emissivity.py (4)
pythermalcomfort/classes_input.py (3)
SkyEmissivityBruntInputs(735-739)SkyEmissivityClarkAllenInputs(751-753)SkyEmissivitySwinbankInputs(743-747)pythermalcomfort/classes_return.py (2)
AutoStrMixin(10-37)Eps_Sky(367-377)pythermalcomfort/utilities.py (2)
Units(36-40)units_converter(786-824)pythermalcomfort/environment/sky_emissivity_2.py (5)
EpsSky(23-49)SkyEmissivity(52-119)brunt(56-73)swinbank(76-94)clark_allen(97-119)
pythermalcomfort/classes_input.py (1)
pythermalcomfort/models/jos3.py (2)
tdb(1403-1414)tdb(1417-1418)
🔇 Additional comments (1)
pythermalcomfort/classes_input.py (1)
133-135: New field tdp: LGTMType and default align with repo patterns. Ensure downstream models validate domains (e.g., T_k > 0) before logs.
Would you like a quick sweep to add T_k > 0 checks where np.log is used?
| @dataclass | ||
| class SkyEmissivityBruntInputs(BaseInputs): | ||
| def __init__(self, tdp): | ||
| super().__init__( | ||
| tdp=tdp, | ||
| ) | ||
|
|
There was a problem hiding this comment.
SkyEmissivity inputs: ctor mismatch and missing validation/docs
- Critical: SkyEmissivityClarkAllenInputs expects tdb but is called with tdp in sky_emissivity_2.py → TypeError.
- Add NumPy‑style docstrings and fcn ∈ [0,1] validation.
@dataclass
class SkyEmissivityBruntInputs(BaseInputs):
- def __init__(self, tdp):
- super().__init__(
- tdp=tdp,
- )
+ """Inputs for the Brunt (1975) sky emissivity model.
+
+ Parameters
+ ----------
+ tdp : float | int | ndarray | list
+ Dew point temperature [°C].
+ """
+ def __init__(self, tdp):
+ super().__init__(tdp=tdp)
@dataclass
class SkyEmissivitySwinbankInputs(BaseInputs):
- def __init__(self, tdb):
- super().__init__(
- tdb=tdb,
- )
+ """Inputs for the Swinbank (1963) model.
+
+ Parameters
+ ----------
+ tdb : float | int | ndarray | list
+ Dry‑bulb temperature [°C].
+ """
+ def __init__(self, tdb):
+ super().__init__(tdb=tdb)
@dataclass
class SkyEmissivityClarkAllenInputs(BaseInputs):
- def __init__(self, tdb, fcn):
- super().__init__(tdb=tdb, fcn=fcn)
+ """Inputs for the Clark & Allen (1978) model.
+
+ Parameters
+ ----------
+ tdp : float | int | ndarray | list
+ Dew point temperature [°C].
+ fcn : float | int | ndarray | list
+ Cloud fraction in [0, 1].
+ """
+ def __init__(self, tdp, fcn):
+ super().__init__(tdp=tdp, fcn=fcn)
+
+ def __post_init__(self):
+ super().__post_init__()
+ fcn = np.asarray(self.fcn, dtype=float)
+ if np.any((fcn < 0.0) | (fcn > 1.0)):
+ raise ValueError("fcn (cloud fraction) must be within [0, 1].")Also applies to: 742-748, 750-754
🤖 Prompt for AI Agents
In pythermalcomfort/classes_input.py around lines 734-740 (and similarly at
742-748 and 750-754), the SkyEmissivity* input classes have constructor
signatures that use tdp instead of tdb (causing a TypeError when
SkyEmissivityClarkAllenInputs is constructed) and lack docstrings and validation
for the fcn parameter; change the constructors to accept tdb (matching callers),
add NumPy-style docstrings for the class/ctor describing parameters (tdb:
dry-bulb temperature, fcn: cloudiness fraction) and behavior, and add runtime
validation that fcn is numeric and within [0, 1] (raise ValueError with a clear
message if not); apply the same fixes to the other two similar class blocks
referenced.
| @dataclass | ||
| class SkyEmissivity: | ||
| """Collection of empirical sky emissivity models.""" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unnecessary @DataClass decorator.
SkyEmissivity contains only static methods and no instance fields, so the @dataclass decorator serves no purpose. Just use a plain class definition.
-@dataclass
class SkyEmissivity:
"""Collection of empirical sky emissivity models."""🤖 Prompt for AI Agents
In pythermalcomfort/environment/sky_emissivity.py around lines 45 to 47, remove
the unnecessary @dataclass decorator from the SkyEmissivity definition and
convert it to a plain class declaration (class SkyEmissivity:), and also remove
the now-unused dataclass import if present; leave the staticmethod definitions
unchanged.
| T_k = tdb_arr + 273.15 | ||
| eps_sky = 9.37e-6 * T_k**2 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating T_k > 0 for robustness.
Although unlikely with realistic temperatures, validating that T_k > 0 after converting to Kelvin would catch extreme edge cases (e.g., tdb < -273.15°C) and align with defensive coding guidelines.
T_k = tdb_arr + 273.15
+ if np.any(T_k <= 0):
+ raise ValueError("tdb must be greater than -273.15 °C to avoid non-physical temperatures.")
eps_sky = 9.37e-6 * T_k**2As per coding guidelines.
🤖 Prompt for AI Agents
In pythermalcomfort/environment/sky_emissivity.py around lines 120-121, after
computing T_k = tdb_arr + 273.15 you must validate that all Kelvin values are >
0 and handle violations; if tdb_arr can be an array, check for any T_k <= 0 and
raise a ValueError (or TypeError if input shape/type is wrong) with a clear
message listing the offending values and recommending valid ranges, otherwise
proceed to compute eps_sky = 9.37e-6 * T_k**2.
FedericoTartarini
left a comment
FedericoTartarini
left a comment
There was a problem hiding this comment.
Hi @benterich,
Thank you so much for working on this. I have left a few comments here and there. I think we should start small and then progressivly add new feature so it will be easier for us to provide comments and reviews in a timely manner.
I’ve built a design option for SkyEmissivity, which can be applied to ground_temperature, sky_temperature, etc. It’s chainable, similar to pandas:
SkyEmissivity.brunt(inputs).apply_dilly(some_inputs)
or
SkyEmissivity.MainMethod().Correction()
I like this approach but I am not very familiar on how to implement it and maintain it. Please review the coderabbit comments which are very useful.
Mrt will use a slightly different pattern because some functions have unique inputs. Using a dictionary with an Enum seems like the simplest, most user-friendly approach. I’ll add more soon to clarify the intentions. Have a look! Let me know what you think. If it works, I’ll add all methods this way.
Some examples will help to better understand how you would like to implement them.
I also suggest adding a docstring to BaseInputs as a description for the variables.
BaseInputs is only used internally in the code, it is not exposed so a docstring is not really needed, we can also create a separate BaseInputs for the MRT so we can keep the code cleaner, MRT function will have very different inputs from the other functions.
Currently, everything is under environment, since these methods are environment-specific and don’t necessarily belong to a model, as discussed our meeting.
Agree
I saw there have been some commits, I can rebase the mrt/development branch after this commit to then fork a more new up to date branch.
Yes, please do that. However, we should try to keep the MRT code as separate as possible so new commits will not affect it.
| @@ -0,0 +1,5 @@ | |||
| """ | |||
| This module handles helper methods to estimate ground temperatures | |||
There was a problem hiding this comment.
please do not include empty files for now, we can add them later when needed, this speeds up the review process
| from enum import Enum | ||
|
|
||
| """ | ||
| This module handles the main Mean Radiant Temperature MRT functions inlcuding helper methods to calucalte longwave radiation |
There was a problem hiding this comment.
this comment is not needed
| class MRTMethod(Enum): | ||
| ForthPower = "4Power" | ||
| NoGeometry = "NoGeometry" | ||
| AnotherDifferentMehtod = "AnotherDifferentMehtod" |
There was a problem hiding this comment.
we should try to use a better naming for the variables, happy to brainstorm
|
|
||
| # MRT methods | ||
| @staticmethod | ||
| def mrt_simple(tdb: float, tr: float, v: float) -> float: |
There was a problem hiding this comment.
please make sure we do not have inputs that are not used in the function. We need to perhaps use a better naming convention.
pythermalcomfort/environment/mrt.py
Outdated
|
|
||
| @staticmethod | ||
| def mrt_asw(asw: float, tdb: float, tr: float) -> float: | ||
| return (asw * tdb + (1 - asw) * tr) |
There was a problem hiding this comment.
Would you like us to help with the formatting of the code? Here for example there are some extra ()
| from dataclasses import dataclass | ||
| import numpy as np | ||
| from typing import Union, Callable, Optional | ||
|
|
There was a problem hiding this comment.
why is this file called _2? We should keep them all in one place
| # ... | ||
|
|
||
| @dataclass(frozen=True) | ||
| class EpsSky: |
There was a problem hiding this comment.
can we use a better naming convention?
| eps_sky: Union[float, np.ndarray] | ||
|
|
||
| @staticmethod | ||
| def dilly(eps: Union[float, np.ndarray]) -> Union[float, np.ndarray]: |
There was a problem hiding this comment.
is using the name of the author the best convention for naming functions?
| """ | ||
| Base inputs with metadata-driven validation. | ||
| Parameters |
There was a problem hiding this comment.
we do not need this docstring
|
|
||
|
|
||
| # ---------------------- | ||
| # Old SkyEmissivity tests |
There was a problem hiding this comment.
no need for these comments
Hi @FedericoTartarini,
Please have a look. I’ve built a design option for SkyEmissivity, which can be applied to ground_temperature, sky_temperature, etc. It’s chainable, similar to pandas:
SkyEmissivity.brunt(inputs).apply_dilly(some_inputs)
or
SkyEmissivity.MainMethod().Correction()
Mrt will use a slightly different pattern because some functions have unique inputs. Using a dictionary with an Enum seems like the simplest, most user-friendly approach. I’ll add more soon to clarify the intentions. Have a look!
Let me know what you think. If it works, I’ll add all methods this way. I also suggest adding a docstring to BaseInputs as a description for the variables.
Currently, everything is under environment, since these methods are environment-specific and don’t necessarily belong to a model, as discussed our meeting.
I saw there have been some commits, I can rebase the mrt/development branch after this commit to then fork a more new up to date branch.
Summary by CodeRabbit
New Features
Tests