Are you sure they can be? Right now the DataStore is only used to store input parameters read by PIMixin and CSVMixin which are all floats. The .parameters() method is also implemented by ModelicaMixin so the return of that will indeed also contain ca.MX.
Are you sure they can be? Right now the DataStore is only used to store input parameters read by PIMixin and CSVMixin which are all floats. The .parameters() method is also implemented by ModelicaMixin so the return of that will indeed also contain ca.MX.
Just a quick reaction with regards to the forecast_index:
There is no requirement that the forecast index coincides with the time of 0 seconds, the forecast index (or more accurately the forecast time) can be different from the reference time used to convert the datetimes to seconds. The current implementations just happen to use the same datetime for both the reference time and the forecast index. The DataStoreAccessor actually already contains a default implementation of initial_time which just returns the time at the forecast index. Though we should check if this is not overridden by some other default in OptimizationProblem / SimulationProblem etc.
As we already mentioned in one of our previous discussions, it may be nicer if (in the future?) the DataStore takes care of storing the reference time used to convert datetimes to seconds and the times may even be offered to the DataStore as a datetimes. When allowing the mixing of PIMixin/CSVMixin etc. we could then allow them to use different forecast indices as long these are stored separately for each variable (optional variable + ensemble_index arguments). It is only the reference time for which they need to use the same, which would be a case of checking whether the DataStore already contains a reference time and if not, setting it, otherwise, use the datetime already there.
@vreeken I've squashed all commits and turned set_timeseries_values and get_timeseries_values into set_timeseries and get_timeseries. This second one now returns a tuple (times, values). The optimization package contains a Timeseries class (timeseries.py) which we may want to move to the data package so it can be returned by get_timeseries instead?
I've removed the set_times method, since the times will now always be set (or checked) with any timeseries. I've kept the get_times method since it can still be used to return the times if all timeseries have them in common and is necessary right now for some methods to be able to verify the times in their input. If we later change the DataStore to allow timeseries with different times we could add optional variable and ensemble_member arguments.
There are some places where get_timeseries is called and the times returned are ignored, obviously this will have to be changed if we want to support different times for different timeseries in the future.
Anne Hommelberg (d05669f0) at 04 Jan 08:52
Add general DataStore and IOMixin classes
Discussion with @vreeken on 3-1-2019:
Other notes: 7. If we're touching the code where the files are read anyway, we might also add functionality to allow timeseries_import_basename to be a list of filenames or a single filename. 8. Might be better to add name-mangling to the input_folder and output_folder variables (__) and add public properties properties. 9. We need to take a hard look at all the methods in DataStore and decide whether they should be turned into properties instead of get... and set....
I will squash the commits into 1 and change the method set_times and set_timeseries_values into a single set_timeseries (which will just call the two existing methods as stated above).
Will close #1007.
Add the option to import and export timeseries data to and from NetCDF.
There are several todos / things to look at in 2019:
This branch should not be merged before merging the io-refactoring branch!
Fixes #1010.
The optimization CSVMixin now supports history, similar to the PIMixin. The forcast date can be set through the csv_forecast_date field. If no forecast date is set, the first time of the input timeseries is used (no history).
Important note: previously the default history was used, which uses the initial_state. Now, by default the first values in the input timeseries are used. Since the initial_state is read from a separate file in the case of the CSVMixin this could result in different values. Should this be changed back to using the initial_state as a default?
@vreeken Yes, !227 (merged) achieves the same thing.
Anne Hommelberg (8ea51e82) at 14 Dec 08:09
Add FutureWarning to get_forecast_index
... and 24 more commits
Anne Hommelberg (d5e73213) at 13 Dec 16:02
Add FutureWarning to get_forecast_index
... and 11 more commits
This refactoring takes care of the bulk of the duplicate code, but there is some code duplication left. For example, both PIMixin and CSVMixin use similar checks to determine whether the input times are sound, both CSVMixins use similar code in read() to read the data from files and store it in the internal data store, etc.
In the future we could look into adding a PIBaseMixin and CSVBaseMixin which can be extended by the PIMixins and CSVMixins respectively. We could also look into adding a IOBaseMixin which can be extended by both IOMixins.
However, for now this is already a major overhaul and improvement of the IO code and will be enough to facilitate easily adding a NetcdfMixin without having add more duplicate code.
Refactoring of the IO mixins (PIMixin and CSVMixin) to (re)move duplicate code.
All IO mixins will now use the same internal data store to store the read timeseries and parameters. This allowed building 2 general IOMixin classes (one for optimization and one for simulation) which contain the methods that where duplicated in both PIMixin and CSVMixin before. Instead of implementing (overriding) these methods themselves, PIMixin and CSVMixin now extend IOMixin. This makes it easier to add new mixins for other file types such as a NetcdfMixin in the future.
self.io.times_sec
vs self.times()
. Especially simulation.check_duplicates
, or make default False?Anne Hommelberg (567b6dfe) at 11 Dec 09:17
Add FutureWarning to get_forecast_index