diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md index c6d297c..7f98e63 100644 --- a/CODE_REVIEW.md +++ b/CODE_REVIEW.md @@ -1,6 +1,6 @@ # Code Review: python_magnetcooling -*Updated after fixes applied through commit `d22efe7` (merge of branch `claude/magnet-cooling-levels-Z2Ab6`).* +*Updated after fixes applied through this branch `claude/update-code-review-PESmk`.* --- @@ -20,71 +20,18 @@ | `_compute_mixed_outlet_temp` unused `pressure` param (issue #7) | `Z2Ab6` branch | Parameter removed; method now takes only `channels` | | Duplicate class definitions in `thermohydraulics.py` (issue #8) | `Z2Ab6` branch | All dataclasses imported from `channel.py`; no local copies | | OOP modules not used by main solver (issue #9) | `Z2Ab6` branch | `ThermalHydraulicCalculator` now routes through `correlations.py` / `friction.py` | -| `fuzzy_factor` incorrectly applied to Dittus/Colburn/Silverberg in `correlations.py` | this branch | `fuzzy` is Montgomery-only; removed `self.fuzzy_factor *` from `DittusBoelterCorrelation`, `ColburnCorrelation`, `SilverbergCorrelation` | - ---- - -## Remaining Bugs - -### 4. `uguess=0` default in `Uw` can cause domain errors (`cooling.py:233`) - -The legacy `Uw` function still has `uguess: float = 0`. `U=0` gives `Re=0`, -and `log(0)` inside `Blasius` or `Filonenko` raises `ValueError`. - -The new OOP path (`ThermalHydraulicCalculator._solve_velocity`) is safe: it -defaults to `velocity_guess: float = 5.0` and guards with -`U = max(velocity_guess, 1e-3)`. However the legacy `cooling.Uw` is still -callable (e.g. from tests or external users) and remains broken at the default. - -Fix: change `uguess: float = 0` → `uguess: float = 1.0`, or add -`U = max(uguess, 1e-3)` before the loop. - -### 5. `compute_from_waterflow` mutates the caller's input (`thermohydraulics.py:248-249`) - -```python -inputs.pressure_inlet = waterflow_params.pressure(current) -inputs.pressure_drop = waterflow_params.pressure_drop(current) -``` - -This silently modifies the caller's `ThermalHydraulicInput` object. A caller -who passes the same `inputs` to multiple `current` values will see stale -pressure values on every call after the first. - -Fix: use `dataclasses.replace(inputs, pressure_inlet=..., pressure_drop=...)` -to work on a copy, or document the mutation prominently in the docstring. +| `fuzzy_factor` incorrectly applied to Dittus/Colburn/Silverberg in `correlations.py` | prior branch | `fuzzy` is Montgomery-only; removed `self.fuzzy_factor *` from `DittusBoelterCorrelation`, `ColburnCorrelation`, `SilverbergCorrelation` | +| `uguess=0` default in `Uw` (issue #4) | this branch | Changed to `uguess: float = 1.0`; added `U = max(uguess, 1e-3)` guard | +| `compute_from_waterflow` mutates caller's input (issue #5) | this branch | Uses `dataclasses.replace(inputs, ...)` to work on a copy | +| Global warning suppression in `waterflow.py` (issue #6) | this branch | Replaced `simplefilter("ignore")` with targeted pint-only filter | +| Dead `importlib_metadata` fallback in `__init__.py` (issue #10a) | this branch | Removed unreachable `except ImportError` branch; project requires Python 3.11+ | +| Dead code in `cooling.py:129-151` (issue #10b) | prior branch | Bare multi-line string of unimplemented friction variants was already removed | +| Commented-out `heatBalance` in `heatexchanger_primary.py` (issue #10c) | this branch | Removed commented-out function | --- ## Remaining Code Quality Issues -### 6. Global warning suppression in `waterflow.py` (`lines 15-19`) - -```python -from warnings import simplefilter -... -simplefilter("ignore") -Quantity([]) -``` - -Suppresses **all** warnings from all libraries at import time of `waterflow`. -This is a broad side-effect that can hide important messages from numpy, -scipy, iapws, and others. - -Fix: -```python -import warnings -warnings.filterwarnings("ignore", category=DeprecationWarning, module="pint") -``` - -### 10. Dead code - -- `__init__.py:37-39` — unreachable `importlib_metadata` fallback (comment - says "Python < 3.8" but the project requires 3.11+; the `except ImportError` - branch can never be reached). -- `cooling.py:129-151` — unimplemented friction variants left as a bare - multi-line string literal (not a docstring, not reachable code). -- `heatexchanger_primary.py:603-605` — commented-out `heatBalance` function. - ### 11. Incomplete test coverage - `test_correlations.py` only covers `MontgomeryCorrelation` and the base @@ -100,16 +47,3 @@ warnings.filterwarnings("ignore", category=DeprecationWarning, module="pint") - A multi-channel integration test for `outlet_temp_mixed` would catch future regressions in the `specific_heat_outlet` / `compute_mixed_outlet_temperature` weighting. - ---- - -## Quick-fix Checklist - -| # | File | Line(s) | Fix | -|---|------|---------|-----| -| 4 | `cooling.py` | 233 | Change `uguess=0` default to `1.0` (or guard `U = max(uguess, 1e-3)`) | -| 5 | `thermohydraulics.py` | 248-249 | Use `dataclasses.replace(inputs, ...)` instead of mutating in-place | -| 6 | `waterflow.py` | 15-19 | Replace `simplefilter("ignore")` with targeted pint filter | -| 10a | `__init__.py` | 37-39 | Remove dead `importlib_metadata` fallback | -| 10b | `cooling.py` | 129-151 | Remove bare multi-line string of unimplemented code | -| 10c | `heatexchanger_primary.py` | 603-605 | Remove commented-out `heatBalance` | diff --git a/python_magnetcooling/__init__.py b/python_magnetcooling/__init__.py index b7b4055..4444a01 100644 --- a/python_magnetcooling/__init__.py +++ b/python_magnetcooling/__init__.py @@ -32,11 +32,7 @@ # Version is read from package metadata (defined in pyproject.toml) # This ensures a single source of truth for the version number -try: - from importlib.metadata import version, PackageNotFoundError -except ImportError: - # Fallback for Python < 3.8 (though we require 3.11+) - from importlib_metadata import version, PackageNotFoundError +from importlib.metadata import version, PackageNotFoundError try: __version__ = version("python-magnetcooling") diff --git a/python_magnetcooling/cooling.py b/python_magnetcooling/cooling.py index 9aea6e7..3a5e975 100644 --- a/python_magnetcooling/cooling.py +++ b/python_magnetcooling/cooling.py @@ -205,7 +205,7 @@ def Uw( friction: str = "Colebrook", Pextra: float = 1.0, fguess: float = 0.055, - uguess: float = 0, + uguess: float = 1.0, rugosity: float = 0.012e-3, ) -> tuple: """ @@ -233,7 +233,7 @@ def Uw( "Swamee": Swamee, } - U = uguess + U = max(uguess, 1e-3) f = fguess isOk = False diff --git a/python_magnetcooling/heatexchanger_primary.py b/python_magnetcooling/heatexchanger_primary.py index 896aacf..7f2dd31 100644 --- a/python_magnetcooling/heatexchanger_primary.py +++ b/python_magnetcooling/heatexchanger_primary.py @@ -612,21 +612,6 @@ def estimate_temperature_elevation( return outlet_temp -# def heatBalance(Tin, Pin, Debit, Power, debug=False): -# """ -# Computes Tout from heatBalance -# -# inputs: -# Tin: input Temp in K -# Pin: input Pressure (Bar) -# Debit: Flow rate in kg/s -# """ -# -# dT = Power / (w.getRho(Tin, Pin) * Debit * w.getCp(Tin, Pin)) -# Tout = Tin + dT -# return Tout - - def calculate_heat_capacity_and_density( Pci: float, Tci: float, Phi: float, Thi: float ) -> Tuple[float, float, float, float]: diff --git a/python_magnetcooling/thermohydraulics.py b/python_magnetcooling/thermohydraulics.py index 0fc9b20..db017d2 100644 --- a/python_magnetcooling/thermohydraulics.py +++ b/python_magnetcooling/thermohydraulics.py @@ -14,6 +14,7 @@ * ``gradHZH`` – same, one h per axial section. """ +import dataclasses from dataclasses import dataclass from math import sqrt from typing import List, Optional, Tuple @@ -245,13 +246,17 @@ def compute_from_waterflow( Returns: Complete thermal-hydraulic solution. """ - inputs.pressure_inlet = waterflow_params.pressure(current) - inputs.pressure_drop = waterflow_params.pressure_drop(current) - + updates = { + "pressure_inlet": waterflow_params.pressure(current), + "pressure_drop": waterflow_params.pressure_drop(current), + } if inputs.cooling_level.is_mean: # Total volumetric flow rate directly from pump curve [m³/s]. - inputs.total_flow_rate = waterflow_params.flow_rate(current) - else: + updates["total_flow_rate"] = waterflow_params.flow_rate(current) + + inputs = dataclasses.replace(inputs, **updates) + + if not inputs.cooling_level.is_mean: # Provide a velocity hint for faster convergence of the grad solver. total_Sh = sum(ch.geometry.cross_section for ch in inputs.channels) Q_total = waterflow_params.flow_rate(current) diff --git a/python_magnetcooling/waterflow.py b/python_magnetcooling/waterflow.py index 82db310..9b95c15 100644 --- a/python_magnetcooling/waterflow.py +++ b/python_magnetcooling/waterflow.py @@ -8,15 +8,15 @@ - Velocity calculations based on operating conditions """ +import warnings from dataclasses import dataclass, field from typing import Optional, List import json import numpy as np -from warnings import simplefilter from pint import UnitRegistry, Quantity -# Ignore pint warnings -simplefilter("ignore") +# Suppress only pint's own DeprecationWarnings, not all library warnings. +warnings.filterwarnings("ignore", category=DeprecationWarning, module="pint") Quantity([]) # Pint configuration