spine-tools / SpineOpt.jl

A highly adaptable modelling framework for multi-energy systems
https://www.tools-for-energy-system-modelling.org/
GNU Lesser General Public License v3.0
53 stars 13 forks source link

Issues with code for computing multi year economic parameter values #1095

Open manuelma opened 1 day ago

manuelma commented 1 day ago
  1. https://github.com/spine-tools/SpineOpt.jl/blob/d74a79799d7ef9aeeb3436ca9b5a79f814e7779f/src/data_structure/economic_structure.jl#L36 does the above work? It seems like we are building a matrix with inconsistent row length?
  2. https://github.com/spine-tools/SpineOpt.jl/blob/d74a79799d7ef9aeeb3436ca9b5a79f814e7779f/src/data_structure/economic_structure.jl#L59 this line has more than 120 characters, we should format the code by introducing a line break, maybe we need a begin block
  3. https://github.com/spine-tools/SpineOpt.jl/blob/d74a79799d7ef9aeeb3436ca9b5a79f814e7779f/src/data_structure/economic_structure.jl#L54 we shouldn't break lines that have less than 120 characters like this, all the arguments should be in the same line
  4. https://github.com/spine-tools/SpineOpt.jl/blob/d74a79799d7ef9aeeb3436ca9b5a79f814e7779f/src/data_structure/economic_structure.jl#L153 we don't need the return, julia functions automatically return the result of the last line which is already the Dict.
  5. Also, for each top level key we have an inner Dict with keys unit, node, and connection. I think it will be better to have unit, node, and connection as top level keys to avoid repetition. In other words, lift unit, node and connection one level up in the Dict structure.
  6. https://github.com/spine-tools/SpineOpt.jl/blob/d74a79799d7ef9aeeb3436ca9b5a79f814e7779f/src/data_structure/economic_structure.jl#L193 why do we want to avoid using collect?

@gnawin here are some comments regarding the code you wrote in case you have time to give it a review.

gnawin commented 1 day ago

Thanks a lot @manuelma. I still have it on my list to go through it again myself. You comments give me some concrete things to check :). Will create a PR after the review.