-
Notifications
You must be signed in to change notification settings - Fork 2
Added OSM road detection feature for consumer selection and simulatio… #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
paulapreuss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really nice already 🚀 Congratulations on your first feature :) I left some comments on some things that could be structured a bit differently, let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's really necessary to create a new file for this code, I would rather integrate it within identify_consumers_on_map.py, rename that file and have it all live within one file instead of multiple very specific ones. What do you think about renaming identify_consumers_on_map.py to osm_utils.py?
offgridplanner/optimization/urls.py
Outdated
| @@ -1,5 +1,6 @@ | |||
| from django.urls import path | |||
|
|
|||
| from . import views | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All functions defined in views are already being imported with the line below, so you don't need this
| initializeMap(); | ||
|
|
||
| map.on("moveend", () => { | ||
| loadAndShowOSMRoads(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am having the issue that if I start looking around the map and exploring (in the case that I was creating a new project, for instance) at some point I run into requests.exceptions.HTTPError: 429 Client Error: Too Many Requests for url: https://overpass-api.de/api/interpreter. I think to avoid this we should not display the roads on map drag but do it like we do with the buildings, where we only load/display the roads once a bbox is selected for the consumers.
| osmRoadsLayer.clearLayers(); | ||
|
|
||
| const geojsonLayer = L.geoJSON(geojson, { | ||
| style: () => ({ color: "#cc99ff", weight: 2, opacity: 0.8 }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
I think it might be a good idea to save the roads data in the database after fetching it once for a project, that way it can just be displayed when loading the consumer selection step or the results page for an existing project without having to worry about the bounding box. The roads data should just be updated when a user creates a new bounding box/consumer selection. For the database, you can use the existing
Essentially, you will need to create a new model |
|
|
||
| if (res.executed) { | ||
| responseMsg.innerHTML = ""; | ||
| Array.prototype.push.apply(road_elements, res.new_roads); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is what's causing your behavior - you are appending the new roads to the road_elements array, so you end up with an array that has both the old and new roads being saved to your database (the saving behavior seems to work fine, it just looks confusing because the old roads are not getting removed). Instead of pushing the new roads to the existing array, you should just overwrite it.
offgridplanner/optimization/views.py
Outdated
| Roads.objects.filter(project=project).delete() | ||
| return JsonResponse({"message": "No data provided"}, status=200) | ||
|
|
||
| df = pd.DataFrame.from_records(road_elements) | ||
| if df.empty: | ||
| Roads.objects.filter(project=project).delete() | ||
| return JsonResponse({"message": "No valid data"}, status=200) | ||
|
|
||
| df = df.drop_duplicates(subset=["road_id"], keep="first") | ||
| required_columns = ["road_id", "coordinates", "how_added", "road_type"] | ||
| df = df[required_columns] | ||
| df["how_added"] = df["how_added"].fillna("automatic") | ||
| df["road_type"] = df["road_type"].fillna("osm") | ||
|
|
||
| Roads.objects.filter(project=project).delete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it seems like you are deleting the existing instance no matter what happens, so no need to do it on each of the conditions :)
offgridplanner/optimization/views.py
Outdated
|
|
||
| Roads.objects.filter(project=project).delete() | ||
|
|
||
| Roads.objects.create(project=project, data=df.to_json(orient="records")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but in general I would overwrite the existing object instead of creating a new one with
roads, created = Roads.objects.get_or_create(project=project) # --> this is a very useful function because it will get you a roads instance regardless of if it already exists or has to be created first
roads.data = df.to_json(orient="records")
roads.save()3adcde8 to
d76cc94
Compare
|
@CelinaKellinghaus please push your latest local changes and also the migration file, then I will take care of rebasing to solve the merge conflicts. When I do that I will have to force push the branch, so you should have no unpushed changes to avoid conflicts or lost code. |
d76cc94 to
36b07e1
Compare
paulapreuss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this PR currently contains reversals for most of the changes that have happened in the repository since the branch for the feature was created. I'm not sure what went wrong with rebasing or if this is just not the most current version? In any case, all the changes that are unrelated to the roads feature need to be taken out of the PR.
| if isinstance(markers_only, str): | ||
| markers_only = True if markers_only == "true" else False # noqa:SIM210 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you deleted this code?
offgridplanner/optimization/views.py
Outdated
| df = df.fillna("null") | ||
| df["latitude"] = df["latitude"].astype(float) | ||
| df["longitude"] = df["longitude"].astype(float) | ||
| df["shs_options"] = df["shs_options"].fillna(0) | ||
| df["custom_specification"] = df["custom_specification"].fillna("") | ||
| df["shs_options"] = df["shs_options"].astype(int) | ||
| df["is_connected"] = df["is_connected"].astype(bool) | ||
| nodes_list = df.to_dict("records") | ||
| is_load_center = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have accidentally brought some deleted code back into this PR when doing the rebase, I see no reason why there should be changes to db_nodes_to_js, as it isn't really relevant to the roads functionality, right?
offgridplanner/optimization/views.py
Outdated
| df, msg = check_imported_consumer_data(df, proj_id) | ||
| df, msg = check_imported_consumer_data(df) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I"m assuming these are also changes that got undone while rebasing
offgridplanner/optimization/views.py
Outdated
| sim_res = data.get("results", {}) | ||
| grid_processor = GridProcessor(proj_id=proj_id, results_json=sim_res.get("grid")) | ||
| results = data.get("results", {}) | ||
| grid_processor = GridProcessor(proj_id=proj_id, results_json=results.get("grid")) | ||
| grid_processor.grid_results_to_db() | ||
| supply_processor = SupplyProcessor( | ||
| proj_id=proj_id, results_json=sim_res.get("supply") | ||
| proj_id=proj_id, results_json=results.get("supply") | ||
| ) | ||
| supply_processor.process_supply_optimization_results() | ||
| supply_processor.supply_results_to_db() | ||
| # Process shared results (after both grid and supply have been processed) | ||
| results = Results.objects.get(simulation__project__id=proj_id) | ||
| results.lcoe_share_supply = ( | ||
| (results.epc_total - results.cost_grid) / results.epc_total * 100 | ||
| ) | ||
| results.lcoe_share_grid = 100 - results.lcoe_share_supply | ||
| assets = ["grid", "diesel_genset", "inverter", "rectifier", "battery", "pv"] | ||
| results.upfront_invest_total = sum( | ||
| [getattr(results, f"upfront_invest_{key}") for key in assets] | ||
| ) | ||
| results.save() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also causing your simulation to fail, since you accidentally deleted a lot of the results processing
offgridplanner/optimization/views.py
Outdated
| project = get_object_or_404(Project, id=proj_id) | ||
| opts = project.options | ||
| res = project.simulation.results | ||
| df = pd.Series(model_to_dict(res)) | ||
| infeasible = bool(df["infeasible"]) if "infeasible" in df else False | ||
| if df.empty: | ||
| return JsonResponse({}) | ||
| # TODO figure out this logic - I changed it so it would run through but it doesnt make so much sense to me | ||
| # if df['lcoe'] is None and opts.do_es_design_optimization is True: | ||
| # return JsonResponse({}) | ||
| # elif df['n_poles']is None and opts.do_grid_optimization is True: | ||
| # return JsonResponse({}) | ||
| if opts.do_grid_optimization is True: | ||
| df["average_length_distribution_cable"] = ( | ||
| df["length_distribution_cable"] / df["n_distribution_links"] | ||
| ) | ||
| df["average_length_connection_cable"] = ( | ||
| df["length_connection_cable"] / df["n_connection_links"] | ||
| ) | ||
| df["gridLcoe"] = float(df["cost_grid"]) / float(df["epc_total"]) * 100 | ||
| else: | ||
| df["average_length_distribution_cable"] = None | ||
| df["average_length_connection_cable"] = None | ||
| df["gridLcoe"] = 0 | ||
| df[["time_grid_design", "time_energy_system_design"]] = df[ | ||
| ["time_grid_design", "time_energy_system_design"] | ||
| ].fillna(0) | ||
| df["time"] = df["time_grid_design"] + df["time_energy_system_design"] | ||
| unit_dict = { | ||
| "n_poles": "", | ||
| "n_consumers": "", | ||
| "n_shs_consumers": "", | ||
| "length_distribution_cable": "m", | ||
| "average_length_distribution_cable": "m", | ||
| "length_connection_cable": "m", | ||
| "average_length_connection_cable": "m", | ||
| "cost_grid": "USD/a", | ||
| "lcoe": "", | ||
| "gridLcoe": "%", | ||
| "esLcoe": "%", | ||
| "res": "%", | ||
| "max_voltage_drop": "%", | ||
| "shortage_total": "%", | ||
| "surplus_rate": "%", | ||
| "time": "s", | ||
| "co2_savings": "t/a", | ||
| "total_annual_consumption": "kWh/a", | ||
| "average_annual_demand_per_consumer": "W", | ||
| "upfront_invest_grid": "USD", | ||
| "upfront_invest_diesel_gen": "USD", | ||
| "upfront_invest_inverter": "USD", | ||
| "upfront_invest_rectifier": "USD", | ||
| "upfront_invest_battery": "USD", | ||
| "upfront_invest_pv": "USD", | ||
| "upfront_invest_converters": "USD", | ||
| "upfront_invest_total": "USD", | ||
| "battery_capacity": "kWh", | ||
| "pv_capacity": "kW", | ||
| "diesel_genset_capacity": "kW", | ||
| "inverter_capacity": "kW", | ||
| "rectifier_capacity": "kW", | ||
| "co2_emissions": "t/a", | ||
| "fuel_consumption": "liter/a", | ||
| "peak_demand": "kW", | ||
| "base_load": "kW", | ||
| "max_shortage": "%", | ||
| "cost_fuel": "USD/a", | ||
| "epc_pv": "USD/a", | ||
| "epc_diesel_genset": "USD/a", | ||
| "epc_inverter": "USD/a", | ||
| "epc_rectifier": "USD/a", | ||
| "epc_battery": "USD/a", | ||
| "epc_total": "USD/a", | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also when we talked about it yesterday on teams it was clear that this code was deleted, from my understanding it was also not there in your local repository, so I'm not quite sure what happened here...
2453524 to
4530e49
Compare
paulapreuss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! 🚀
I know that this was a comprehensive feature with some challenges, but you pushed through it :) I think it works and looks very nice, thank you for implementing this. Feel free to merge the PR.

This PR adds the ability to display main roads from OpenStreetMap on the maps used in both
the consumer selection and simulation results pages.
Features:
Closes #50