oemof / DHNx

District heating system optimisation and simulation models
MIT License
27 stars 12 forks source link

Clean up OSM import #47

Closed jnnr closed 3 years ago

jnnr commented 3 years ago

This PR addresses #40, moving the functionality of the osm example to an importer class.

Open TODOs

jnnr commented 3 years ago

You are right. This does not change optimization or simulation models. There are still some steps missing to use it in a workflow with optimization and simulation. This would be the next steps - using the tools and find out how the different parts can interplay with another.

joroeder commented 3 years ago

I am wondering, is geopandas so far a requirement? Yes, but only for the osmnx import, right? And besides osmnx, if we want to have geojson Importer Classes we need also geopandas, right? But for the simulation and optimisation part, there is no need for geopandas at the moment, right? I think the osmnx import is very powerful, and maybe everybody, who is doing something with district heating systems in python, needs at some point geopandas. So, in the end, we will have geopandas anyway, right? What do you think?

jnnr commented 3 years ago

Geopandas is in "extras_require" in setup.py. It is needed for osm import but not for optimisation and simulation. As soon as someone writes a geojson importer, we can add a try import except to the input_output module.

jnnr commented 3 years ago

Regarding the remaining problem of the edges that are not divided when new nodes are introduced: The problem is here

https://github.com/oemof/DHNx/blob/dev/dhnx/dhn_from_osm.py#L82

New edges are just the old edges combined with the new, connecting edges. One step needs to be added: Cutting the edges in two at the new nodes. Can someone help?

joroeder commented 3 years ago

Geopandas is in "extras_require" in setup.py. It is needed for osm import but not for optimisation and simulation. As soon as someone writes a geojson importer, we can add a try import except to the input_output module.

Sounds good. However, at the moment it is in install_requires - I am confused ;-)

New edges are just the old edges combined with the new, connecting edges. One step needs to be added: Cutting the edges in two at the new nodes. Can someone help?

Do you want to have this in the first release? I think, of course, this would be nice. But on the other hand, we can always add this features later. The import so far shows at least the principle workflow, and for the second release, this can be done unhurriedly.

jnnr commented 3 years ago

Geopandas is in "extras_require" in setup.py. It is needed for osm import but not for optimisation and simulation. As soon as someone writes a geojson importer, we can add a try import except to the input_output module.

Sounds good. However, at the moment it is in install_requires - I am confused ;-)

You are right. osmnx is in extras_require. Geopandas is a dependency of osmnx. So geopandas could be removed from install_requires, it seems. Should test if that works though.

New edges are just the old edges combined with the new, connecting edges. One step needs to be added: Cutting the edges in two at the new nodes. Can someone help?

Do you want to have this in the first release? I think, of course, this would be nice. But on the other hand, we can always add this features later. The import so far shows at least the principle workflow, and for the second release, this can be done unhurriedly.

In my view, it is a bug. But the dev has the bug as well, so we could still merge this PR if everything else looks good.

jnnr commented 3 years ago

I moved geopandas to extras require. With this change, the users can decide for themselves to install it. If they do not install it, some examples will not work, but the import error is caught and a message is printed.

jnnr commented 3 years ago

In my view this is ready. The linters' warnings are dealt with in the branch of the simulation models, so they should be resolved when we first merge #42.