spine-tools / Spine-Database-API

Database interface to Spine generic data model
https://www.tools-for-energy-system-modelling.org/
GNU Lesser General Public License v3.0
6 stars 5 forks source link

Merge indexed parameter values in import operations #152

Closed manuelma closed 1 year ago

manuelma commented 2 years ago

I want to run SpineOpt in a loop in toolbox, where each iteration processes a different day. But when writing time-series results to the output db, each iteration of the loop overwrites the previous result, and at the end I just get the last day's result in my output DB.

I think it would make sense to merge indexed values (eg time-series) rather than overwriting when import. Because in general, import_data always tries to 'update' values when possible.

Re https://github.com/Spine-project/spine-engine/issues/47

DillonJ commented 2 years ago

Didn't we address this in https://github.com/Spine-project/SpineOpt.jl/issues/410?

I guess this is API functionality - we should make sure that both of these ways play nicely with the other

manuelma commented 2 years ago

It's not the same, Spine-project/SpineOpt.jl#410 is for a single SpineOpt run. This is for independent runs that are linked together in a toolbox loop, so each run writes independently to the db. I don't think there's any overlapping here.

jkiviluo commented 2 years ago

We've also discussed the ability to choose whether to 1) overwrite, 2) append or 3) fail in case there is existing data (not just time series). I was trying to look for an issue, but didn't find it. There is a related issue: https://github.com/Spine-project/Spine-Toolbox/issues/1091.

manuelma commented 2 years ago

Thanks @jkiviluo that's a good point.

I will add the following three options to the importer: keep, replace, and merge, so the user can select from the UI (default to merge).

One question is whether we want the behavior to be mapping-level or importer-level, what do you think?

jkiviluo commented 2 years ago

It probably is an importer level choice most of the time, but of course there can be use cases where you'd like to be more precise. However, there a decent work-around (make multiple importers), so it's ok to go importer-level.

Furthermore, this is one those options that often should not be in the repository (other than default) - user could be changing this depending on the circumstance. We don't have a solution for that problem yet: https://github.com/Spine-project/Spine-Toolbox/issues/1238

soininen commented 2 years ago

Wouldn't this be useful feature for Combiner/Merger as well?

manuelma commented 2 years ago

Wouldn't this be useful feature for Combiner/Merger as well?

Yes, I will add it there too.

jkiviluo commented 2 years ago

BTW, I think 'keep' and 'merge' are ambiguous. Keep could either mean 'do nothing' or 'append'. I think 'append' is unambiguous. Meanwhile 'Merge' does not define what do you do when merging: 'replace' or 'append'. That's why I suggested 'overwrite', 'append' and 'fail'.

Now I think there is also a fourth case: 'purge first' but that may be harder to define. As our existing 'purge' shows, there are multiple levels of purge.

Also, if you are combining, order needs to be established (possibly including the target DB).

manuelma commented 2 years ago

We can discuss the wording but I think 'keep existing', 'replace', and 'merge' are standard terms to deal with file conflicts, I borrowed the terms from there. 'Append' is problematic since it is generally interpreted as adding stuff at the end regardless of repeated indexes, and that's not what we want to do here.

manuelma commented 2 years ago

Now I think there is also a fourth case: 'purge first'

I think purge options should be a separate set of options, not part of the conflict resolution.

Also, if you are combining, order needs to be established (possibly including the target DB).

Yes that's important but I think this issue 'covers' it, https://github.com/Spine-project/Spine-Toolbox/issues/1091 (hopefully one day we'll have time to do it).

jkiviluo commented 1 year ago

@manuelma I'm closing this as stale. Re-open if you see there is something still here.