search

oemof / DHNx

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

Features/move gistools to dhnx #65

Closed joroeder closed 3 years ago

joroeder commented 3 years ago

Related issue: #54

jnettels commented 3 years ago

Just FYI: While not an error, I have encountered an unclosed file ResourceWarning when running the example "import_osm_invest.py". It seems to be related to osmnx and I have opened an issue there: https://github.com/gboeing/osmnx/issues/607

jnettels commented 3 years ago

I hope you don't mind, I added a tiny commit d104ab6 with additional info for configuring the cbc solver. This took me a while to get to work and should be useful here. I would also like to properly set up the logging module for the "gistools" code (as noted here #69), but that does not need to happen in this PR.

joroeder commented 3 years ago

Yes, thank you. On the one hand, it is something solver-specific. But on the other hand, you are right, it might be very helpful and also shows how to apply and hand these options to the solver ...

jnettels commented 3 years ago

Sorry, I rely on Spyder's code style check, but flake8 is even pickier than that...

joroeder commented 3 years ago

Okay, this one is now finished from my side, I gave the last finishing touches from my side after standing still for so long. This is almost a shame, as this PR contains such valuable steps for setting up the network from your individual gis data.

I also included the most important hints (e.g. to the example showing the whole model chain from osm to optimisation results) in a docs chapter: https://dhnx.readthedocs.io/en/features-move_gistools_to_dhnx/geometry_preparation.html

@jnnr It would be great if you could have a look at it. Since it is mainly additional code, I do not expect that there are major flaws concerning the existing functionalities of the package. I would appreciate if we could merge this one soon ...

joroeder commented 3 years ago

Thank you for your response!

On a more general level: It would be good to describe the PR a bit better. What problem does it solve and how? Which problems does it not solve?

Yes, sorry, I agree ;-) Please see below. I thought that we have talked about that, but nontheless, you are right.

How do the functions introduced here stand in relation to the one in the module dhn_from_osm, containing a function connect_points_to_network? Is it replaced by the new functions?

Yes, this would be the next step and fix the bug that the lines are not split in the function connect_points_to_network. But therefore, I found it convenient to first fix the existing osm import (#64) and provide our existing approach (this PR) in the DHNx package, we are currently using in our projects. And then, we should use a separate Issue/PR for replacing the old function.

Will the new functions be integrated in the OSMNetworkImporter in the future? The Importer class has the benefit that it wraps the different steps that are performed in the example import_osm_invest and hides the details of the implementation, which makes it easier to use and to maintain.

Yes, this could/should be integrated in future (I will open a new issue for that). However, the design philosophy differs and needs some discussion and work to integrate it properly (This is also the reason for not doing it directly with this PR. For example, it was not intended to wrap everything, because the users should be able to use their own buildings layer and eg only the network from osm. Also, the proceeding differs at many points in detail. I also think that by using and already providing the current approach, we can figure out the best way to improve the NetworkImporter – but all this can be discussed in future issues/PR)