Fix PV potential calculation based on tilt angle #3763
Conversation
…area_FIX DE_Ns_CONSTRUCTION_STANDARD.xlsx_fix
…sults-summary Update scripts.yml
Indoor_comfort temperature swapped
Changed the details to include the published paper and update authors.
Update README.md with DOI
If we keep both there are chances that one is updated and the other isn't
the comment makes it seem like the calculation is hard coded for the northern hemisphere, but it actually doesn't matter for the calculation whether it's north or south (since calc_optimal_angle returns an absolute value)
also cleaning up unnecessary code and correcting equation 5.4.2
Complete use type properties fixed, adapted to the reference
|
Wow - this sounds great by just reading at your text @martin-mosteiro |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cea/technologies/solar/photovoltaic.py (2)
167-171: LGTM: Azimuth angle sign convention aligns with Duffie (2013).The adjustment of azimuth angle sign convention (south = 0°, east negative, west positive) correctly follows the standard convention in Duffie (2013).
Consider using a ternary operator for more concise code:
-if latitude >= 0: - Az = solar_properties.Az - 180 # south is 0°, east is negative and west is positive (p. 13) -else: - Az = solar_properties.Az # north is 0° +Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Az # south is 0° for northern hemisphere🧰 Tools
🪛 Ruff (0.8.2)
168-171: Use ternary operator
Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Azinstead ofif-else-block(SIM108)
206-210: Enhancement: Improved angle of incidence calculation using pvlib.The switch to
pvlib.irradiance.aoiimproves accuracy and maintainability by using a well-tested library implementation.The commented-out code can be safely removed since it's replaced by the pvlib implementation.
cea/technologies/solar/photovoltaic_thermal.py (1)
178-182: LGTM: Consistent azimuth angle convention.The azimuth angle adjustment matches the implementation in photovoltaic.py, maintaining consistency.
Consider using a ternary operator for more concise code:
-if latitude >= 0: - Az = solar_properties.Az - 180 # south is 0°, east is negative and west is positive (p. 13) -else: - Az = solar_properties.Az # north is 0° +Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Az # south is 0° for northern hemisphere🧰 Tools
🪛 Ruff (0.8.2)
179-182: Use ternary operator
Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Azinstead ofif-else-block(SIM108)
cea/technologies/solar/solar_collector.py (1)
764-768: LGTM: Consistent azimuth angle convention across solar modules.The azimuth angle adjustment maintains consistency with photovoltaic.py and photovoltaic_thermal.py.
Consider using a ternary operator for more concise code:
-if latitude_deg >= 0: - Az = solar_properties.Az - 180 # south is 0°, east is negative and west is positive (p. 13) -else: - Az = solar_properties.Az # north is 0° +Az = solar_properties.Az - 180 if latitude_deg >= 0 else solar_properties.Az # south is 0° for northern hemisphere🧰 Tools
🪛 Ruff (0.8.2)
765-768: Use ternary operator
Az = solar_properties.Az - 180 if latitude_deg >= 0 else solar_properties.Azinstead ofif-else-block(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cea/technologies/solar/photovoltaic.py(12 hunks)cea/technologies/solar/photovoltaic_thermal.py(4 hunks)cea/technologies/solar/solar_collector.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
cea/technologies/solar/solar_collector.py
765-768: Use ternary operator Az = solar_properties.Az - 180 if latitude_deg >= 0 else solar_properties.Az instead of if-else-block
(SIM108)
cea/technologies/solar/photovoltaic_thermal.py
179-182: Use ternary operator Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Az instead of if-else-block
(SIM108)
cea/technologies/solar/photovoltaic.py
168-171: Use ternary operator Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Az instead of if-else-block
(SIM108)
203-203: Local variable teta_z_rad is assigned to but never used
Remove assignment to unused variable teta_z_rad
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: windows-latest
- GitHub Check: macos-latest
- GitHub Check: ubuntu-latest
🔇 Additional comments (3)
cea/technologies/solar/photovoltaic.py (1)
326-326: Fixed equation 5.4.2 for diffuse radiation calculation.The correction aligns with Duffie and Beckman (2013) by using the correct coefficient in the equation.
cea/technologies/solar/photovoltaic_thermal.py (1)
233-235: Enhancement: Consistent use of pvlib for angle calculations.The switch to
pvlib.irradiance.aoimaintains consistency with photovoltaic.py.cea/technologies/solar/solar_collector.py (1)
776-777: Enhancement: Standardized angle calculations using pvlib.The switch to
pvlib.irradiance.aoicompletes the standardization of angle calculations across all solar technology modules.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cea/utilities/solar_equations.py (1)
634-639: Good addition of special case for flat panels!The code correctly handles flat panels by setting azimuth to 0° since orientation doesn't matter when tilt is 0°.
Consider using a ternary operator for more concise code:
- if B != 0: - teta_z = degrees(asin(xdir / sin(B))) # surface azimuth before adjusting for sign convention - else: - # for flat panels, surface azimuth doesn't matter - teta_z = 0 + # surface azimuth before adjusting for sign convention (0° for flat panels as azimuth doesn't matter) + teta_z = degrees(asin(xdir / sin(B))) if B != 0 else 0🧰 Tools
🪛 Ruff (0.8.2)
634-638: Use ternary operator
teta_z = degrees(asin(xdir / sin(B))) if B != 0 else 0instead ofif-else-block(SIM108)
cea/technologies/solar/photovoltaic.py (1)
167-172: Excellent fix for azimuth sign convention!The code now correctly implements the Duffie (2013) convention:
- Northern hemisphere: south is 0°, east is negative, west is positive
- Southern hemisphere: north is 0°
This resolves the core issue where tilted panels were yielding less electricity than flat panels.
Consider using a ternary operator for more concise code:
- if latitude >= 0: - Az = solar_properties.Az - 180 # south is 0°, east is negative and west is positive (p. 13) - else: - Az = solar_properties.Az # north is 0° + # Adjust azimuth: In northern hemisphere, south is 0° (east negative, west positive) + # In southern hemisphere, north is 0° + Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Az🧰 Tools
🪛 Ruff (0.8.2)
168-171: Use ternary operator
Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Azinstead ofif-else-block(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cea/technologies/solar/photovoltaic.py(12 hunks)cea/utilities/solar_equations.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
cea/technologies/solar/photovoltaic.py
168-171: Use ternary operator Az = solar_properties.Az - 180 if latitude >= 0 else solar_properties.Az instead of if-else-block
(SIM108)
203-203: Local variable teta_z_rad is assigned to but never used
Remove assignment to unused variable teta_z_rad
(F841)
cea/utilities/solar_equations.py
634-638: Use ternary operator teta_z = degrees(asin(xdir / sin(B))) if B != 0 else 0 instead of if-else-block
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: windows-latest
- GitHub Check: macos-latest
- GitHub Check: ubuntu-latest
🔇 Additional comments (5)
cea/utilities/solar_equations.py (3)
239-244: Documentation improvements look good!The added comments clearly explain the filtering conditions for sensor points and hours with low radiation potential.
399-402: Documentation improvement enhances understanding!The added comment correctly explains that panels facing the equator (surface azimuth = 0° or 180°) yield identical results, which is physically accurate.
529-529: Good fix to include 0° tilt angle!The condition now correctly includes perfectly flat panels (0°) in the first category, which aligns with the PR objectives.
cea/technologies/solar/photovoltaic.py (2)
326-326: Good fix for equation 5.4.2!The implementation now correctly matches Duffie and Beckman (2013), and the variable naming is more consistent.
496-502: Documentation improvements enhance code understanding!The assumptions about tilt angles are now clearly documented:
- Tilted roof: panel tilt matches roof tilt
- Wall: 90° tilt
- Flat roof: tilt determined by Quinn et al. method
…veloper-version Remove developer version in Windows setup
DE use_types schedule and properties fix
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cea/databases/DE/documentation.md (4)
158-161: Clarify Residential Profiles Statement.
The statement “Cooling is always equal to CH database. The naming convention is the same as the CH database. For residential profiles:” appears repetitive. Consider rephrasing for clarity and conciseness, for example: “For residential profiles, cooling settings follow the CH database convention.”
163-171: Enhance Clarity in the Non-Residential Types Section.
The newly added “#### Non-Residential types” section explains the heterogeneous data basis well. However, in line 167 the phrase “taken into account” is a bit wordy. Consider simplifying it to “considered” or “factored in” for improved readability. Also, ensure that the transition and context regarding the use of DIN/TS 18599 and Standardlastprofile des BDEW are clearly highlighted.[style]
🧰 Tools
🪛 LanguageTool
[style] ~167-~167: ‘taken into account’ might be wordy. Consider a shorter alternative.
Context: ...fferent thermal zones of a building are taken into account. For this reason, the values cannot be ...(EN_WORDINESS_PREMIUM_TAKEN_INTO_ACCOUNT)
185-186: Review Heating Setpoints Detail.
The entries forThs_set_C(setpoint temperature for heating) andThs_setb_C(setback temperature for heating) now include detailed references to DIN standards. Please verify that these descriptions and numerical guidelines align with DIN EN 16798-1 and DIN V 18599-10. Consider streamlining the language if possible to enhance readability.
195-195: Correct Spelling in Scheduling Description.
The row on scheduling currently reads:| OCCUPANCY | modified according to DIN EN 16798 and DIN18599 for Residential. Non residential assumed equal to CH. |To maintain consistency and proper compound word formatting, “Non residential” should be hyphenated as “Non-residential”.
-| Non residential assumed equal to CH. +| Non-residential assumed equal to CH.🧰 Tools
🪛 LanguageTool
[misspelling] ~195-~195: This expression is normally spelled as one or with a hyphen.
Context: ... EN 16798 and DIN18599 for Residential. Non residential assumed equal to CH. | | APPLIENCES | 0...(EN_COMPOUNDS_NON_RESIDENTIAL)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
cea/databases/DE/archetypes/use_types/FOODSTORE.csvis excluded by!**/*.csvcea/databases/DE/archetypes/use_types/KINDERGARDEN.csvis excluded by!**/*.csvcea/databases/DE/archetypes/use_types/OFFICE.csvis excluded by!**/*.csvcea/databases/DE/archetypes/use_types/OFFICE_1P.csvis excluded by!**/*.csvcea/databases/DE/archetypes/use_types/SEMINAR.csvis excluded by!**/*.csvcea/databases/DE/archetypes/use_types/SENIOR_RES.csvis excluded by!**/*.csvcea/databases/DE/archetypes/use_types/USE_TYPE_PROPERTIES.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (2)
cea/databases/DE/documentation.md(3 hunks)setup/cityenergyanalyst.nsi(0 hunks)
💤 Files with no reviewable changes (1)
- setup/cityenergyanalyst.nsi
🧰 Additional context used
🪛 LanguageTool
cea/databases/DE/documentation.md
[style] ~167-~167: ‘taken into account’ might be wordy. Consider a shorter alternative.
Context: ...fferent thermal zones of a building are taken into account. For this reason, the values cannot be ...
(EN_WORDINESS_PREMIUM_TAKEN_INTO_ACCOUNT)
[misspelling] ~195-~195: This expression is normally spelled as one or with a hyphen.
Context: ... EN 16798 and DIN18599 for Residential. Non residential assumed equal to CH. | | APPLIENCES | 0...
(EN_COMPOUNDS_NON_RESIDENTIAL)
[typographical] ~199-~199: If you want to indicate numerical ranges or time ranges, consider using an en dash.
Context: ...599-10:2018 Tab. 4 Nutzungszeit von 6:00 - 23:00 | | SETBACK | if not SETPOINT| ...
(DASH_RULE)
🔇 Additional comments (2)
cea/databases/DE/documentation.md (2)
177-177: Verify Fresh Water Consumption Metric.
The table row forVw_ldpnow describes the peak specific fresh water consumption as “average water consumption for Germany [^BDEW]”. Please confirm that this reference and description are consistent with other parts of the documentation, and that the value accurately reflects the data source.
409-409: New Author Added.
The addition of “Urbano Tataranni” to the Authors list is correctly positioned in alphabetical order. This inclusion enhances the representation of contributions.
This commit fixes a small bug leading to reduced distances between PV panel rows on the southern hemisphere and negative PV surface areas for latitudes that experience full or near polar nights.
|
Thanks so much @MatNif for the review! He caught (and fixed) some important issues that only popped up when looking at higher latitudes (such as Trondheim) that caused negative spacing between panels and negative array areas. In the process, I have also added another fix to an equation that only applied to zenith angles of less than 70°, so that now we have a working equation for higher zenith angles. This PR looks mostly okay now, pending the tests (which are currently broken due to the update in the input file format in PR #3766). After the tests pass, I suggest we run a few more edge cases (e.g., far north, far south, far east and far west) to make sure the PV calculations are accurate and then we'll be ready to merge! |
|
@martin-mosteiro unit tests should be fixed now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cea/technologies/solar/photovoltaic_thermal.py (1)
186-188: Remove commented out code.These angle calculations are no longer needed since we're using
pvlib. Keeping commented code can lead to confusion and maintenance issues.- # lat_rad = radians(latitude) - # g_rad = np.radians(solar_properties.g) - # ha_rad = np.radians(solar_properties.ha)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/technologies/solar/photovoltaic_thermal.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: windows-latest
- GitHub Check: macos-latest
- GitHub Check: ubuntu-latest
🔇 Additional comments (3)
cea/technologies/solar/photovoltaic_thermal.py (3)
16-16: LGTM! Good choice of dependencies.The addition of
pvliband the function rename improve code quality:
pvlibis a well-maintained library for solar modeling- The rename from
calc_properties_PV_dbtoget_properties_PV_dbbetter reflects the function's purposeAlso applies to: 23-23
43-43: LGTM! Improved geographical accuracy.The addition of the longitude parameter and updated function call enhance the solar calculations:
- Longitude is essential for accurate solar position calculations
- Function call correctly updated to match the renamed import
Also applies to: 77-77
178-183: LGTM! Critical fix for azimuth angle calculations.This change resolves the inconsistent sign convention for azimuth angles:
- Correctly implements Duffie and Beckman (2013) convention
- Handles both hemispheres appropriately (north: south=0°, east=-ve, west=+ve; south: north=0°)
- Fixes the issue where tilted panels incorrectly yielded less electricity than flat panels
|
Hi @martin-mosteiro The "0" degree flat all ran through for four cases: pv_north (very north in Sweden), pv_ch (zurich), pv_sg (singapore), pv_south (very south in Chile, even more south than argentina hehe). However, except for the Singapore case worked as intended. All the three had some issues. For Scenario pv_north, your script triggered "'GB not in expected range'". I had some hacky fixed as the lastest commit and it worked. The results are as intended. For Scenario pv_ch, "optimal" yields much less electricity than "flat". For Scenario pv_south, 1 degree tilted yields less than 0 degree and optimal is much less than 0-degree. I am attaching the scenario I was using via a drop box link, which I sent to you via Slack. Feel free to take a look. Or am I testing them wrongly? Just drop me a message if you need a quick chat over this. Thank you. |
|
@ShiZhongming Thanks for your review! Indeed, the "optimal" angle in CEA means maximum kWh/m2 PV NOT maximum total PV production (that's the name it had before already btw – not really my preference ;)). Counterintuitive – I believe @MatNif rephrased the description in the GUI to make the naming a little more sane. I'll check all the other cases you mentioned over the coming days! |
|
Hi @martin-mosteiro |
This PR fixes issue #3282. As discussed there (and in #3281), tilted PV panels were producing less electricity than flat ones, even at the optimal tilt angle. After investigating, I realized there was an inconsistent sign convention in the azimuth angle between the geographic coordinates and the ones used by the PV model (from Duffie and Beckman, 2013). After fixing this issue, you indeed get the expected improvement in PV performance as a function of tilt angle. In the process of fixing this issue, I have also found an incorrect constant (K=4 m^-1 from Duffie and Beckman was incorrectly coded as
K=0.4 #m^-1) and one equation that inexplicably didn't match Duffie and Beckman (2013) (eq. 5.4.2 had a factor of59.68instead of 59.7), with marginal effect on the results. This PR fixes those as well. Finally, I have also changed the solar equations so an angle of 0° (flat placement) is possible.Before fixing the issue, here are the results for different PV tilt angles for one case study building:
After fixing the issue, here are the results for the same PV tilt angles for the same case study building:
I have also fixed the azimuth issue on the PVT calculation. I am not sure why the calculation for PVT used a legacy function for angle of incidence (previously used for PV until @shanshanhsieh changed it in 2021), so I've changed it to
pvlib.irradiance.aoito match the PV calculation. After doing so, the PVT potential goes from 134.4 kWh/m2 at 10° and 101.22 kWh/m2 at 30° to 158.27 kWh/m2 at 10° and 171.2 kWh/m2 at 30°.I have done the same for the angle of incidence for the SC calculation.
How to test this PR
Summary by CodeRabbit
Release Notes
New Features
pvliblibrary for improved solar panel performance modeling.Bug Fixes
Documentation
custom-tilt-angleparameter.Refactor