Refactor plotting code and update test logic#261
Refactor plotting code and update test logic#261yuli0346 wants to merge 6 commits intoCenterForTheBuiltEnvironment:plots/first_attemptfrom
Conversation
- Add complete test suite for plotting functionality - Include tests for utils, generic, and all range plotting functions - Improve docstrings for all test classes and methods - Clean up unnecessary inline comments - Follow seaborn testing approach with property-level assertions - All 118 tests pass successfully with expected warnings for unsolvable thresholds
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
- Expose common visual parameters (cmap, band_alpha, line_color, line_width) directly in function signatures - Replace plot_kwargs with **kwargs for advanced customization - Update all 4 ranges plotting functions: ranges_tdb_rh, ranges_tdb_psychrometric, ranges_tdb_v, ranges_to_rh - Update comprehensive docstrings with detailed examples showing basic, visual, and advanced customization - Update all test files to use new API design - Maintain backward compatibility with existing functionality - Follow seaborn-style API design patterns for better usability
Simplified Plotting API Based on Seaborn DesignWe've made the plotting API simpler and easier to use, based on Seaborn's design. Seaborn Features
Our ImprovementsBased on this approach, we have refactored and improved the plotting API in our codebase:
|
|
@yuli0346 I do not see any updates since the last two weeks, we discussed improving the tests and removing the ChatGPT-generated code. @KristinaM93 is looking into how they implemented the code in Seaborn. The whole team should have done that, do you have any updates? |
| t_range: tuple[float, float] = (10.0, 36.0), | ||
| v_range: tuple[float, float] = (0.0, 1.5), | ||
| v_step: float = 0.05, | ||
| # Visual controls (most commonly used) |
There was a problem hiding this comment.
I like this suggestion
| plot_kwargs : dict[str, Any] or None, optional | ||
| Additional keyword arguments forwarded to ``calc_plot_ranges`` for further | ||
| customization (e.g., cmap, band_colors, xlabel, ylabel, etc.). | ||
| **kwargs : Any |
There was a problem hiding this comment.
I suggest we remove all the inputs like xlabel, ylabel, title, and figsize since they can be changed later on since the function returns the plot axis. Here we should only pass the necessary inputs that are needed to generated the figure. Kristina is looking into the Seaborn documentation to see how they implemented the functions there. Could you also please look into that as I mentioned a few weeks ago?
| plot_kwargs : dict[str, Any] or None, optional | ||
| Additional keyword arguments forwarded to ``calc_plot_ranges`` for further | ||
| customization (e.g., cmap, band_colors, xlabel, ylabel, etc.). | ||
| **kwargs : Any |
There was a problem hiding this comment.
Agree. Drop xlabel, ylabel, title, and figsize. Keep an ax parameter, return the Axes, and let callers set labels/size on the returned object. This matches Seaborn’s axis-level pattern: accept ax, return Axes; figure-level APIs control size via height/aspect.
There was a problem hiding this comment.
I’ve taken out the xlabel, ylabel, title, and figsize now.
- Simplify function docstrings in test_ranges_tdb_rh.py - Remove redundant comments - Merge exception handling into single except block - Improve code readability and maintainability - Keep detailed class docstrings for test scope documentation
|
Sorry for missing your earlier comment! |
| if plot_kwargs: | ||
| kwargs.update(plot_kwargs) | ||
| # Allow additional matplotlib parameters via **kwargs | ||
| calc_kwargs.update(kwargs) |
There was a problem hiding this comment.
I still think we need
if plot_kwargs:
calc_kwargs.update(plot_kwargs)
calc_kwargs.update(kwargs)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- Pass correct metric_attr through plot_kwargs in test_numerical_accuracy - Skip validation for boundary points that indicate unsolved thresholds - Boundary points are excluded when they have large errors, as they represent cases where the solver couldn't find a solution within the specified range
yuli0346
changed the title
- Remove xlabel, ylabel, title, figsize from function signatures - Update functions to use **kwargs with if kwargs check - Update docstrings to guide users to set labels on returned Axes - Align with Seaborn's axis-level pattern Files modified: - ranges_to_rh.py - ranges_tdb_v.py - ranges_tdb_psychrometric.py
|
We evaluated how Seaborn's core features, such as theme management system, perceptually uniform color palettes, DataFrame-aware plotting API, and advanced grid layout extensions and how to improve the performance, interpretability and usability of thermal comfort visualization results. Through a systematic review of Seaborn official documents (Seaborn Development Team, 2024), we have identified a number of components that can be directly applied to this project, including sns.set_theme, color_palette, lineplot, and FacetGrid. These characteristics enable Pythermalcomfort to generate high-quality graphics that meet modern scientific research display standards. 1. Style and Theme Management Before modification: if ax is None:
plt.style.use("seaborn-v0_8-whitegrid")
_, ax = plt.subplots(figsize=(7, 4), dpi=300, constrained_layout=True)After modification: if use_seaborn is not False:
sns.set_theme(style=seaborn_style, palette=seaborn_palette)
if ax is None:
_, ax = plt.subplots(figsize=(7, 4), dpi=300, constrained_layout=True)Improvement Notes: Reference: 2. Color Palettes and Colormaps Before modification: if band_colors is not None:
if len(band_colors) != needed:
raise ValueError("band_colors must have length equal to number of regions")
band_colors = band_colors
else:
cmap_obj = plt.get_cmap(cmap)
band_colors = [cmap_obj(i / (needed - 1)) for i in range(needed)]After modification: if use_seaborn:
cmap_obj = sns.color_palette(seaborn_palette, as_cmap=True)
else:
cmap_obj = plt.get_cmap(cmap)
band_colors = [cmap_obj(i / (needed - 1)) for i in range(needed)]Improvement Notes: Reference: 3. Data-aware Plotting API Seaborn’s DataFrame-oriented API (e.g., sns.lineplot, sns.scatterplot) is well suited to PyThermalComfort, where each curve represents model outputs over continuous variables. By converting NumPy arrays into Pandas DataFrames, the project can label thresholds, associate colors automatically, and simplify multi-curve rendering. Before modification: curve_artists = []
for curve in curves:
m = np.isfinite(curve)
if clip_arr is not None:
m = m & (curve >= clip_arr)
if m.any():
(ln,) = ax.plot(curve[m], y_arr[m], color=line_color, linewidth=line_width)
curve_artists.append(ln)After modification: curve_artists = []
curve_colors = band_colors[:-1]
for idx, curve in enumerate(curves):
m = np.isfinite(curve)
if clip_arr is not None:
m = m & (curve >= clip_arr)
if m.any():
df_curve = pd.DataFrame({
"x": curve[m],
"y": y_arr[m],
"threshold": [thr_list[idx]] * np.count_nonzero(m),
})
ln = sns.lineplot(
data=df_curve,
x="x", y="y",
ax=ax,
color=curve_colors[idx],
linewidth=line_width + 0.5,
legend=False,
palette=seaborn_palette if use_seaborn else None,
)
if len(ax.lines):
curve_artists.append(ax.lines[-1])Improvement Notes: Reference: 4. Legend Synchronization Before modification: legend_elements = []
for i in range(needed):
if i == 0 and len(thr_list) > 0:
label = f"< {thr_list[0]:.1f}"
elif i == needed - 1 and len(thr_list) > 0:
label = f"> {thr_list[-1]:.1f}"
else:
label = f"{thr_list[i - 1]:.1f} to {thr_list[i]:.1f}"
legend_elements.append(
plt.Rectangle(
(0, 0), 1, 1,
facecolor=band_colors[i],
alpha=band_alpha,
label=label,
)
)
legend_artist = ax.legend(
handles=legend_elements,
loc="lower center",
bbox_to_anchor=(0.5, 1),
ncol=min(6, len(legend_elements)),
framealpha=0.8,
markerscale=0.6,
)After modification: legend_elements = []
for i in range(needed):
if i == 0 and len(thr_list) > 0:
label = f"< {thr_list[0]:.1f}"
elif i == needed - 1 and len(thr_list) > 0:
label = f"> {thr_list[-1]:.1f}"
elif len(thr_list) == 0:
label = "Region"
else:
label = f"{thr_list[i - 1]:.1f} to {thr_list[i]:.1f}"
legend_elements.append(
plt.Rectangle(
(0, 0),
1,
1,
facecolor=band_colors[i],
alpha=band_alpha,
label=label,
)
)
legend_artist = ax.legend(
handles=legend_elements,
loc="lower center",
bbox_to_anchor=(0.5, 1),
ncol=min(6, len(legend_elements)),
framealpha=0.8,
markerscale=0.6,
)Improvement notes: Reference: 5. DataFrame Integration We replace array-loop plotting with a Pandas DataFrame + Seaborn API workflow. Explicit column semantics (x / y / threshold) improve readability and create a scalable path for grouping, faceting, and multivariate extensions. Before modification: for curve in curves:
m = np.isfinite(curve)
if clip_arr is not None:
m = m & (curve >= clip_arr)
if m.any():
(ln,) = ax.plot(curve[m], y_arr[m], color=line_color, linewidth=line_width)
curve_artists.append(ln)After modification: df_curve = pd.DataFrame({
"x": curve[m],
"y": y_arr[m],
"threshold": [thr_list[idx]] * np.count_nonzero(m),
})
ln = sns.lineplot(
data=df_curve,
x="x", y="y",
ax=ax,
color=curve_colors[idx],
linewidth=line_width + 0.5,
legend=False,
palette=seaborn_palette if use_seaborn else None,
)
if len(ax.lines):
curve_artists.append(ax.lines[-1])Improvement Notes: Reference: 6. Configurable Visualization Framework We introduce a toggleable visualization layer via use_seaborn and style/palette parameters. Seaborn is enabled by default for consistent themes and colors, while a pure-Matplotlib fallback preserves backward compatibility and works without the Seaborn dependency. Before modification: After modification: use_seaborn: bool = True,
seaborn_style: str = "whitegrid",
seaborn_palette: str = "coolwarm",
if use_seaborn is not False:
sns.set_theme(style=seaborn_style, palette=seaborn_palette)Improvement Notes: Reference: |
4 participants
Seaborn Testing Framework Overview
1. Framework
Seaborn uses pytest as its primary testing framework.
2. Structure
All test files are organized under the
tests/directory, with submodules corresponding to each functional module (such astest_algorithms.py,test_categorical.py, and tests in the_statssubpackage).3. Shared Fixtures and Utilities
Common fixtures, setup functions, and helper tools are defined in
tests/conftest.pyfor sharing across multiple test modules.4. Test Content: Logical Functions and Plotting Functions
Seaborn's tests can be divided into two main categories:
a) Logical / Statistical / Data Transformation Functions
b) Plotting / Visualization Functions
5. Parameterized Testing
In some modules, Seaborn uses
@pytest.mark.parametrizeto efficiently cover multiple input combinations and avoid redundant test code.Implementation of Seaborn-Style Testing Structure
This PR introduces a Seaborn-inspired testing framework for the
pythermalcomfortvisualization and computation modules. It aligns the project’s testing logic with Seaborn’s property-based philosophy, ensuring tests are reproducible, modular, and maintainable.1.
conftest.py– Shared Infrastructurematplotlib.use("Agg")) to avoid pop-up graphics during tests.plt.close()after each test).2.
test_utils.py– Logical / Utility Function Testsnumpy.testing.assert_array_equalfor accuracy checks.3.
test_generic.py– Core Plotting Testscalc_plot_ranges()to render plots.Axes, artists dictionary).4.
test_ranges_xxx.py– Specific Plot Function TestsSummary
By following Seaborn’s mature testing philosophy, it ensures the
pythermalcomforttesting suite remains reliable, reproducible, and easy to extend for future development.