fix: implement dash-leaflet #235, address #272#276
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR integrates Leaflet-based interactive maps into the location selection page. It introduces a complete GeoJSON data pipeline, a Flask serving endpoint, dash-leaflet rendering with custom marker tooltips, and updates modal interactions to work with the new map click data structure. ChangesDash-Leaflet Map Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Thanks Tobias,
it looks good! I could not test it but hopefully this will be faster to load and easier to navigate.
Now we just need to add a search bar to put the name of the city/weather station the user is looking for ;)
Best,
Giovanni
…________________________________
From: Toby Kramer ***@***.***>
Sent: 21 May 2026 4:35 PM
To: CenterForTheBuiltEnvironment/clima ***@***.***>
Cc: giobetti ***@***.***>; Mention ***@***.***>
Subject: [CenterForTheBuiltEnvironment/clima] fix: implement dash-leaflet #235, address #272 (PR #276)
Hi @FedericoTartarini<https://github.com/FedericoTartarini>,
I implemented #235<#235> as a fix for #272<#272> and the remaining error from #273<#273> where the select page took ages to load. Using Dash-Leaflet and GeoJSON, everything seems to load much faster (at least locally).
This is a PR into development. Test it and see how you like it. I made minor tweaks to the default cluster colors (Sorry, I had to add a CSS file 😜) to match the overall tool UI.
Using this new way of visualizing weather files on the map, along with updating our repository of OneBuilding climate file URLs I did two weeks ago, we’ll have almost double the weather files in Clima with this update (compared to 2022 when we last pulled from OneBuilding).
Note: There are about 18k future weather files for Canada (around 32x per location with different iterations). I added a one-liner to filter them out for now. This issue<#275> describes why. I think at this point this is the best solution to avoid confusion.
Let me know if you have any questions! PR summary below✌🏼
@stefanoschiavon<https://github.com/stefanoschiavon> @giobetti<https://github.com/giobetti>
PR Summary
* Replaced px.scatter_mapbox (Plotly) with dash-leaflet dl.GeoJSON(cluster=True) on the select page. The previous approach serialised all ~100K location markers into a single Plotly figure JSON on every page visit (~50 MB); the new approach serves a static, browser-cached gzip file (~1.5 MB) and renders only the visible clusters at any zoom level (= much, much faster)
* Added scripts/build_locations_geojson.py to pre-process both source files (epw_location.json + one_building.csv) into a single assets/data/locations.geojson.gz. Future-climate scenario files (_Future/ URLs, ~18K rows (Canada only) are excluded for now and will be handled in a dedicated feature.
* As before, all marker tooltips show name, lat/lon, and source for all locations; OneBuilding markers additionally show period, elevation, time zone, 99% Heating DB, and 1% Cooling DB.
* Two marker colours are preserved: purple (#3a0ca3) for EnergyPlus, blue (#4895ef) for OneBuilding.
* I tried to align the cluster colors with overall tool looks
Use
* Select page map loads immediately on / with clustered markers visible
* Zooming in breaks clusters into individual points with correct colours
* Hovering a point shows the full tooltip
* Clicking a point opens the confirmation modal with the location name
* Clicking Yes downloads the EPW and enables all other tabs as before
* Uploading a local EPW file still works
Screenshot.2026-05-21.at.4.11.38.PM.png (view on web)<https://github.com/user-attachments/assets/3f3f8ef0-ea25-43ee-8187-ec9b93cd847b>
________________________________
You can view, comment on, or merge this pull request online at:
#276
Commit Summary
* 9e8febb<9e8febb> fix: implement dash-leaflet #235, address #272
File Changes
(9 files<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files>)
* M Pipfile<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-230078d672f10d17463a8a6265cad825b790885898256a3365be90685caac58d> (1)
* M Pipfile.lock<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-a86c67a0a29ed0e95909b9b7c420140f302d17399ee6dcce4e1a51a14d27fd51> (44)
* M app.py<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-568470d013cd12e4f388206520da39ab9a4e4c3c6b95846cbc281abc1ba3c959> (12)
* A assets/cluster.css<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-55a3dc2dd6dd139f4d495f5098af831c8a34d9f29edc88bf5f3ded77d5164550> (6)
* A assets/dashExtensions_default.js<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-fbb2e4ed6775290d624e52d8820d0b7c29506a03d4b0042bf3b28910d9086e36> (33)
* A assets/data/locations.geojson.gz<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-652e15214980322e05cc1bed7062a62d9d051eaeaba3019a6271c2b477bac108> (0)
* M pages/lib/global_element_ids.py<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-4704862b016148d93c4f32f94bb9e94ee2efb23d9576842933c4fac7698482b7> (1)
* M pages/select.py<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-65910ce6ea0cd4bd224cfcd46939d0b3e9db79d419c9e05c2ab9b6fb052be00f> (151)
* A scripts/build_locations_geojson.py<https://github.com/CenterForTheBuiltEnvironment/clima/pull/276/files#diff-71515013efb949b76416ccbd8ebf18fac4f363df2ee5312b21d2b0ac47ed0dc5> (88)
Patch Links:
* https://github.com/CenterForTheBuiltEnvironment/clima/pull/276.patch
* https://github.com/CenterForTheBuiltEnvironment/clima/pull/276.diff
—
Reply to this email directly, view it on GitHub<#276>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AIWZJAE6DBFTUQZ2SARVXXD434H3JAVCNFSM6AAAAACZHZB2VSVHI2DSMVQWIX3LMV43ASLTON2WKOZUGQ4TKNBRGEZDENY>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@t-kramer great work! Greenlight from me. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
I love the new implementation. Fantastic work!!!! Even if you used CSS ;) While loading the application locally I have noticed the following issue as you can see some of the circles overlap can we fix that or shall we ignore that? I guess that we can fix it by extending the range of a grouping so two groups are not so close. I'm going to review the rest of the code now. I've also asked code rabbit to do a review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.py`:
- Around line 15-27: The endpoint serve_locations_geojson currently returns
Cache-Control: no-cache which forces revalidation; update the response headers
returned by serve_locations_geojson to use cache-friendly directives for a
static, versioned gzipped asset (e.g., "Cache-Control": "public,
max-age=31536000, immutable") while keeping "Content-Encoding": "gzip" (and
leave conditional send_file behavior as-is) so browsers/CDNs can aggressively
cache the file.
In `@pages/select.py`:
- Around line 34-36: The module-level expression that builds _GEO_URL calls
os.path.getmtime at import time and will raise if
"assets/data/locations.geojson.gz" is missing; wrap that logic (the _GEO_URL
construction) in a safe check or try/except: first check os.path.exists(...) or
catch FileNotFoundError around os.path.getmtime and fall back to a safe default
(e.g. omit the ?v= param or use 0) so importing the module won't fail in
fresh/dev environments; update the code that references _GEO_URL to rely on this
guarded value.
- Around line 40-60: The tooltip HTML in _point_to_layer (created via assign)
directly concatenates unescaped feature.properties values (e.g., p.title,
p.period, p.elev, p.tz, p.heat99, p.cool1) and can lead to HTML injection; fix
by escaping these property strings before building the html string (or using a
safe DOM/text API) and then pass the sanitized string to marker.bindTooltip;
locate the assign-created function named _point_to_layer and ensure every
insertion of p.<...> is sanitized (e.g., replace <, >, &, " and ' with HTML
entities) before concatenation.
In `@Pipfile`:
- Line 20: Update the Pipfile to pin dash-leaflet to a Dash 3–compatible release
(replace dash-leaflet = "*" with dash-leaflet == "1.1.3" or another 1.1.x+
version) and then regenerate Pipfile.lock so the lockfile and Pipfile agree
(current Pipfile.lock uses 1.0.15 which is pre-Dash-3); target dash-leaflet
1.1.3 to align with Dash 3 compatibility and run pipenv lock to produce an
updated lockfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b966501-4f00-4f8b-ab11-1ec3fe66545a
⛔ Files ignored due to path filters (2)
Pipfile.lockis excluded by!**/*.lockassets/data/locations.geojson.gzis excluded by!**/*.gz
📒 Files selected for processing (8)
Pipfileapp.pyassets/cluster.cssassets/dashExtensions_default.jspages/lib/global_element_ids.pypages/select.pyscripts/build_locations_geojson.pytests/test_select.py
💤 Files with no reviewable changes (1)
- pages/lib/global_element_ids.py
| @app.server.route("/geojson/locations") | ||
| def serve_locations_geojson(): | ||
| return ( | ||
| send_file( | ||
| "assets/data/locations.geojson.gz", | ||
| mimetype="application/json", | ||
| as_attachment=False, | ||
| download_name="locations.geojson", | ||
| conditional=True, | ||
| ), | ||
| 200, | ||
| {"Content-Encoding": "gzip", "Cache-Control": "no-cache"}, | ||
| ) |
There was a problem hiding this comment.
Use cache-friendly headers for the GeoJSON endpoint.
Line 26 sets Cache-Control: no-cache, which forces revalidation and undercuts the map-load performance gain from serving a static gzipped asset with versioned URLs.
💡 Suggested change
`@app.server.route`("/geojson/locations")
def serve_locations_geojson():
return (
send_file(
"assets/data/locations.geojson.gz",
mimetype="application/json",
as_attachment=False,
download_name="locations.geojson",
conditional=True,
),
200,
- {"Content-Encoding": "gzip", "Cache-Control": "no-cache"},
+ {
+ "Content-Encoding": "gzip",
+ "Cache-Control": "public, max-age=31536000, immutable",
+ },
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.py` around lines 15 - 27, The endpoint serve_locations_geojson currently
returns Cache-Control: no-cache which forces revalidation; update the response
headers returned by serve_locations_geojson to use cache-friendly directives for
a static, versioned gzipped asset (e.g., "Cache-Control": "public,
max-age=31536000, immutable") while keeping "Content-Encoding": "gzip" (and
leave conditional send_file behavior as-is) so browsers/CDNs can aggressively
cache the file.
| _GEO_URL = "/geojson/locations?v=" + str( | ||
| int(os.path.getmtime("assets/data/locations.geojson.gz")) | ||
| ) |
There was a problem hiding this comment.
Guard _GEO_URL generation against missing asset at import time.
Line 35 calls os.path.getmtime(...) during module import. If the file is absent in a fresh/dev environment, the whole page import fails.
🛠️ Suggested fallback
-_GEO_URL = "/geojson/locations?v=" + str(
- int(os.path.getmtime("assets/data/locations.geojson.gz"))
-)
+try:
+ _geo_mtime = int(os.path.getmtime("assets/data/locations.geojson.gz"))
+except OSError:
+ _geo_mtime = 0
+_GEO_URL = f"/geojson/locations?v={_geo_mtime}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pages/select.py` around lines 34 - 36, The module-level expression that
builds _GEO_URL calls os.path.getmtime at import time and will raise if
"assets/data/locations.geojson.gz" is missing; wrap that logic (the _GEO_URL
construction) in a safe check or try/except: first check os.path.exists(...) or
catch FileNotFoundError around os.path.getmtime and fall back to a safe default
(e.g. omit the ?v= param or use 0) so importing the module won't fail in
fresh/dev environments; update the code that references _GEO_URL to rely on this
guarded value.
| _point_to_layer = assign("""function(feature, latlng, ctx) { | ||
| const p = feature.properties; | ||
| const color = p.source === "ep" ? "#3a0ca3" : "#4895ef"; | ||
| const marker = L.circleMarker(latlng, { | ||
| radius: 5, color: color, fillColor: color, fillOpacity: 0.8, weight: 1 | ||
| }); | ||
|
|
||
| let html = '<b>' + (p.title || '') + '</b><br/>' | ||
| + 'Lat: ' + latlng.lat.toFixed(2) + ', Lon: ' + latlng.lng.toFixed(2) + '<br/>'; | ||
| if (p.source === 'ob') { | ||
| html += 'Period: ' + (p.period || 'N/A') + '<br/>' | ||
| + 'Elevation: ' + (p.elev || 'N/A') + ' m<br/>' | ||
| + 'Time zone: GMT' + (p.tz || 'N/A') + '<br/>' | ||
| + '99% Heating DB: ' + (p.heat99 || 'N/A') + '<br/>' | ||
| + '1% Cooling DB: ' + (p.cool1 || 'N/A') + '<br/>' | ||
| + 'Source: Climate.OneBuilding.Org'; | ||
| } else { | ||
| html += 'Source: EnergyPlus'; | ||
| } | ||
| marker.bindTooltip(html, {sticky: true, opacity: 0.9}); | ||
| return marker; |
There was a problem hiding this comment.
Escape tooltip content before injecting into HTML.
Lines 47–55 concatenate unescaped feature.properties values into HTML. If any upstream value contains HTML, this can inject markup into the tooltip.
🔐 Suggested hardening
_point_to_layer = assign("""function(feature, latlng, ctx) {
const p = feature.properties;
+ const esc = (v) => String(v ?? "")
+ .replace(/&/g, "&")
+ .replace(/</g, "<")
+ .replace(/>/g, ">")
+ .replace(/"/g, """)
+ .replace(/'/g, "&`#39`;");
const color = p.source === "ep" ? "`#3a0ca3`" : "`#4895ef`";
@@
- let html = '<b>' + (p.title || '') + '</b><br/>'
+ let html = '<b>' + esc(p.title || '') + '</b><br/>'
+ 'Lat: ' + latlng.lat.toFixed(2) + ', Lon: ' + latlng.lng.toFixed(2) + '<br/>';
if (p.source === 'ob') {
- html += 'Period: ' + (p.period || 'N/A') + '<br/>'
- + 'Elevation: ' + (p.elev || 'N/A') + ' m<br/>'
- + 'Time zone: GMT' + (p.tz || 'N/A') + '<br/>'
- + '99% Heating DB: ' + (p.heat99 || 'N/A') + '<br/>'
- + '1% Cooling DB: ' + (p.cool1 || 'N/A') + '<br/>'
+ html += 'Period: ' + esc(p.period || 'N/A') + '<br/>'
+ + 'Elevation: ' + esc(p.elev || 'N/A') + ' m<br/>'
+ + 'Time zone: GMT' + esc(p.tz || 'N/A') + '<br/>'
+ + '99% Heating DB: ' + esc(p.heat99 || 'N/A') + '<br/>'
+ + '1% Cooling DB: ' + esc(p.cool1 || 'N/A') + '<br/>'
+ 'Source: Climate.OneBuilding.Org';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pages/select.py` around lines 40 - 60, The tooltip HTML in _point_to_layer
(created via assign) directly concatenates unescaped feature.properties values
(e.g., p.title, p.period, p.elev, p.tz, p.heat99, p.cool1) and can lead to HTML
injection; fix by escaping these property strings before building the html
string (or using a safe DOM/text API) and then pass the sanitized string to
marker.bindTooltip; locate the assign-created function named _point_to_layer and
ensure every insertion of p.<...> is sanitized (e.g., replace <, >, &, " and '
with HTML entities) before concatenation.
| dash-iconify = "*" | ||
| scipy = "==1.12.0" | ||
| kgcpy = "*" | ||
| dash-leaflet = "*" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest stable dash-leaflet release, and which versions are compatible with Dash 3.x?
💡 Result:
Latest stable dash-leaflet release: 1.1.3 [1][2]. Dash 3.x compatibility: - dash-leaflet 1.1.3 declares a dependency constraint of dash >=3.0.0, so it is compatible with Dash 3.x releases [1][2]. - The dash-leaflet project also documents that versions 1.1.1+ were released as “Dash 3 compatible” and notes a specific incompatibility issue related to Dash 3 / React 18.3 that was addressed via updates [3]. Practical takeaways: - Use dash-leaflet==1.1.3 with Dash 3.x (>=3.0.0) [1][2].
Citations:
- 1: https://pypi.org/project/dash-leaflet/
- 2: https://pypi.org/project/dash-leaflet/1.1.3/
- 3: Incompatible with dash=3 and REACT >= 18.3 emilhe/dash-leaflet#276
🏁 Script executed:
rg -n "dash-leaflet\s*=|\[packages\]|\[dev-packages\]|^dash\s*=" -S .Repository: CenterForTheBuiltEnvironment/clima
Length of output: 188
🌐 Web query:
dash-leaflet latest version PyPI and compatibility with dash 3.x
💡 Result:
Latest dash-leaflet version on PyPI (as of 2026-05-26): 1.1.3 [1]. Compatibility with Dash 3.x: - dash-leaflet 1.1.3 declares a dependency on dash >= 3.0.0, so it is intended to work with Dash 3.x (and will not allow Dash 2.x) [1][2]. - The dash-leaflet 1.1.3 metadata also indicates Python >= 3.11 [1][2]. - Dash 3.0 is documented by Plotly as removing/altering some older component stubs and includes breaking changes; if your app uses any libraries/components that Dash 3 requires to be updated, you may need to update those libraries too [3]. Recommended install (to match the intended constraints): - pip install "dash>=3" "dash-leaflet==1.1.3" [1][2].
Citations:
- 1: https://pypi.org/project/dash-leaflet/
- 2: https://pypi.org/project/dash-leaflet/1.1.3/
- 3: https://dash.plotly.com/dash-3-0-migration
🏁 Script executed:
ls -laRepository: CenterForTheBuiltEnvironment/clima
Length of output: 1897
🏁 Script executed:
rg -n "dash-leaflet" -S .Repository: CenterForTheBuiltEnvironment/clima
Length of output: 155
🏁 Script executed:
python3 - <<'PY'
import itertools, re, sys
path="Pipfile.lock"
start=200
end=260
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f,1):
if start<=i<=end:
print(f"{i}:{line.rstrip()}")
PYRepository: CenterForTheBuiltEnvironment/clima
Length of output: 2832
🌐 Web query:
dash-leaflet 1.0.15 dependency on dash (Dash >=?) PyPI
💡 Result:
dash-leaflet 1.0.15 declares a dependency on Dash with the constraint dash >=2.13.0 on PyPI. [1] So the answer to “Dash >=?” is: Dash >= 2.13.0. [1]
Citations:
🌐 Web query:
dash-leaflet 1.0.15 Dash 3 compatibility
💡 Result:
Dash-leaflet 1.0.15 is not considered “Dash 3 compatible” on its own. Instead, Dash 3 compatibility was implemented starting from dash-leaflet 1.1.0 (and related 1.1.x releases), while 1.0.15 only declares compatibility with Dash >=2.13.0. Evidence: - The PyPI page for dash-leaflet 1.0.15 shows a dependency constraint of dash >=2.13.0 (no Dash 3-specific minimum is stated for 1.0.15). [1] - The project changelog/compare history between 1.0.15 and 1.1.0 explicitly notes “Working in Dash 3 compatibility” and “Update dependency to Dash 3.0.0” as part of the 1.1.0 change. [2] - There is a GitHub issue reporting incompatibility when using dash=3 with dash-leaflet (and discussing fixes that were released in later 1.1.x versions), indicating that older 1.0.x builds can be problematic with Dash 3. [3] Practical takeaway: - If you need Dash 3 compatibility, use dash-leaflet 1.1.0 or later (for example, 1.1.3). [2][4] Install guidance: - pip install dash>=3 dash-leaflet>=1.1.0
Citations:
- 1: https://pypi.org/project/dash-leaflet/1.0.15/
- 2: emilhe/dash-leaflet@1.0.15...1.1.0
- 3: Incompatible with dash=3 and REACT >= 18.3 emilhe/dash-leaflet#276
- 4: emilhe/dash-leaflet@1.1.2...1.1.3
Pin dash-leaflet and align it with Dash 3.2.
Pipfileusesdash-leaflet = "*"(drift risk);Pipfile.lockcurrently pinsdash-leaflet==1.0.15.dash-leaflet==1.0.15is not the Dash 3–compatible line (Dash 3 compatibility started indash-leaflet1.1.0+). Upgrade to a Dash 3–compatible version (e.g.,dash-leaflet==1.1.3) and regeneratePipfile.lock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Pipfile` at line 20, Update the Pipfile to pin dash-leaflet to a Dash
3–compatible release (replace dash-leaflet = "*" with dash-leaflet == "1.1.3" or
another 1.1.x+ version) and then regenerate Pipfile.lock so the lockfile and
Pipfile agree (current Pipfile.lock uses 1.0.15 which is pre-Dash-3); target
dash-leaflet 1.1.3 to align with Dash 3 compatibility and run pipenv lock to
produce an updated lockfile.
|
@t-kramer Could you please fix the issue from code rabbit and the issue identified by Claude below? Some are repeated. I also formatted the .js code with Prettier and pushed the changes.
{"Content-Encoding": "gzip", "Cache-Control": "no-cache"} The URL already has a mtime-based cache buster (?v=), meaning the URL changes every time the file is rebuilt. This makes the resource effectively immutable per URL. Using no-cache forces the browser to revalidate on every request, throwing away the benefit. Change to: {"Content-Encoding": "gzip", "Cache-Control": "public, max-age=31536000, immutable"}
assign() from dash_extensions.javascript generates/updates assets/dashExtensions_default.js at runtime. Committing a hand-written version of that file alongside the assign() call means the two can diverge, and the runtime-generated version will silently overwrite the committed one (or vice versa depending on the Dash version). Either remove the committed JS file and rely solely on assign(), or remove the assign() call and load the JS directly — don't maintain both.
for feat in epw_data["features"][:2585]: This is unexplained. Extract it as a named constant at the top of the file with a comment explaining why that specific count is used (e.g. matching the old behaviour, or excluding a known bad tail).
Both app.py ("assets/data/locations.geojson.gz") and pages/select.py (os.path.getmtime("assets/data/locations.geojson.gz")) use relative paths. These break if the process working directory is ever not the repo root. Use Path(file).parent / "assets" / "data" / "locations.geojson.gz" for robustness.
dash-leaflet = "*" The lock file pins 1.0.15, which is good, but the wildcard in Pipfile means pipenv update could silently pull a breaking major release. Pin to at least ">=1.0.15,<2".
Both new files are missing a trailing newline. Minor, but inconsistent with the rest of the repo. Minor Notes
|
FedericoTartarini
left a comment
FedericoTartarini
left a comment
There was a problem hiding this comment.
@t-kramer Great work again. I left a few comments in line with the text. Please tag me once you have addressed my comments and the comments from Claude and Code Rabbit so I can do a second review. You can close open comments as you address them. Thank you so much for working on this.
| # for individual point features, never for cluster markers, so properties are always complete. | ||
| _point_to_layer = assign("""function(feature, latlng, ctx) { | ||
| const p = feature.properties; | ||
| const color = p.source === "ep" ? "#3a0ca3" : "#4895ef"; |
There was a problem hiding this comment.
The color string is duplicated in here and in dashExtension_defaults.py. Could you please check for code duplication, not only in the color string, but in other parts of the code? Perhaps we can export them and save them as a constant and import them in both files.
| @@ -0,0 +1,51 @@ | |||
| window.dashExtensions = Object.assign({}, window.dashExtensions, { | |||
| default: { | |||
| function0: function (feature, latlng, ctx) { | |||
There was a problem hiding this comment.
function0 is not used and can be removed.
| if (p.source === "ob") { | ||
| html += | ||
| "Period: " + | ||
| (p.period || "N/A") + |
There was a problem hiding this comment.
PyCharm is complaining that this may not be defined, the same for the other attributes of p. Can we give a more meaningful name to P? Perhaps we can call it "Properties of XXX"
| default: { | ||
| function0: function (feature, latlng, ctx) { | ||
| const p = feature.properties; | ||
| const color = p.source === "ep" ? "#3a0ca3" : "#4895ef"; |
There was a problem hiding this comment.
Checking that the source is equal to "ep" which is a string, it is quite dangerous because we may miss this in the future, can we find a more elegant solution? for example to define a class with the two different sources.
| @@ -0,0 +1,97 @@ | |||
| """ | |||
| Build assets/data/locations.geojson.gz from the two source data files. | |||
There was a problem hiding this comment.
Add a bit more information to the docstring at the top so we know what this file is for and also describe it in the readme.
|
@t-kramer So sorry for tagging you again but I found a few more issue. I am also tagging @stefanoschiavon and @giobetti to know their opinion.
|
|
@FedericoTartarini, four of your comments point to the same issue: we’re using longitude and latitude to define the location for the weather file, which often overlaps since we have multiple files for the same location. Dash Leaflet solves this by stacking them and exposing them in a “flower” shape, but it’s not ideal because the flower only appears when clicking on the group, not when zooming in. I’ll check for a built-in fix or settings we can adjust. Alternatively, we could introduce “jitter” by slightly adjusting the coordinates to keep files at roughly the same location, like SFO, but not directly on top of each other. I’ll report back on our options. But as I said in the PR, this is a problem we will encounter much more frequently in the future (see my example with Canadian future weather files with x34 versions for each location), so I think we should find a robust solution. I don't consider filtering or only displaying a selection of weather files a good solution long-term. I also agree with your other comments on limiting zoom. I’ll look into that too. |
|
@t-kramer I like adding a bit of noise. I think I was already doing this in the past. I agree that manually filtering is not a good solution and I definitely don't know how to solve the issue, but at the same time, providing 12 weather files for a single location is going to be very confusing for our users. They will start wondering how much they differ? Do I need to click on all of them and compare them? For future weather files, the explanation and the inclusion is a bit easier, because if we can show the different periods and the climate model in the tooltip, then it's going to be easier to select one versus the other. But the distinction is not very clear for historical data, which do not show the reference period. |
|
Dear toby, dear Federico,
Thank you so much for the great work here.
I agree with Federico that we do not need to show 12 files for location as it will only be confusing for the user. My question is if we have a clear set of criteria for us to choose which of the 12 files to display.
Regarding the overlapping files: jitter is the way Federico solved this in the past. It’s a neat solution for historic data. If we want to add future data, the mechanism would need to be slightly different because it would be a bit strange to be looking for future data around the site. I actually quite like the “flower” but it should automatically appear at a specific zoom level and on click. Then each petal could be colored to show if it links to a historical or future weather file (and also what type of future weather file if we have more than one).
Message ID: ***@***.***>
Hope this helps,
Best,
Giovanni
|
|
Thanks for sharing, I agree. Let me think about inclusion/exclusion criteria for historical weather data. Starting by filtering out files with lots of missing metadata (e.g. reference period) might be a good option. I'll think about it and get back to you! |
4 participants
Hi @FedericoTartarini,
I implemented #235 as a fix for #272 and the remaining error from #273 where the select page took ages to load. Using Dash-Leaflet and GeoJSON, everything seems to load much faster (at least locally).
This is a PR into
development. Test it and see how you like it. I made minor tweaks to the default cluster colors (Sorry, I had to add a CSS file 😜) to match the overall tool UI.Using this new way of visualizing weather files on the map, along with updating our repository of OneBuilding climate file URLs I did two weeks ago, we’ll have almost double the weather files in Clima with this update (compared to 2022 when we last pulled from OneBuilding).
Note: There are about 18k future weather files for Canada (around 32x per location with different iterations). I added a one-liner to filter them out for now. This issue describes why. I think at this point this is the best solution to avoid confusion.
Let me know if you have any questions! PR summary below✌🏼
@stefanoschiavon @giobetti
PR Summary
Use
Summary by CodeRabbit
New Features
Tests