RTCTOOLS-543: overhaul of PIMixIn - [merged] #1163

Closed SGeeversAtVortech closed 2 weeks ago

In GitLab by @OJMvD on Nov 18, 2016, 13:54

Merges RTCTOOLS-543 -> master

PIMixin: removed support for using a timeseries_export.xml placeholder file. This file is always entirely constructed by RTC-Tools 2. This also means that we have removed support for interpolation of the output series (this was based on the times in the output placeholder). This should be done in FEWS.

The MixIn now outputs all variables in the output_variables list to the timeseries_export, if the variable has a location and parameter mapping in rtcDataConfig. If not, the variable is skipped.

Furthermore, the mixin of course passes all relevant information from the import timseries and rtcDataConfig to the export file (times, start/end date, location, parameter, etc.) to be able to construct a fully filled TimeSeries object.

Added that the timezone of the import file is read and transferred to the export file.

Added proper support for writing ensemble values (previously this did not function as intended).

Added that the forecast datetime is writting in the export file if it was present in the import file.

Added mapping of RTCTools variable to FEWS location/parameter/qualifiers (the reverse was already present).

In GitLab by @OJMvD on Nov 18, 2016, 13:56

Nosetests that use PIMixIn run fine with this new code. Some nosetests fail, control_tree I think, but that is due to some bug in other code.

Apart from that, a lot has changed/been added, so I can imagine some finetuning of the code is still needed :)

In GitLab by @baayen on Nov 21, 2016, 14:28

Which ones fail for you? For me, they all pass.

In GitLab by @baayen on Nov 21, 2016, 14:29

Commented on rtctools/optimization/pi_mixin.pyx line 362

Why do we want this logic here, rather than in PI.Timeseries.ensemble_size?

In GitLab by @baayen on Nov 21, 2016, 14:30

Commented on rtctools/optimization/pi_mixin.pyx line 281

Replace " -1 >=" with ">".

In GitLab by @baayen on Nov 21, 2016, 14:33

Commented on rtctools/data/rtc.pyx line 110

Would be good to use a namedtuple here, so that we can access fields using attribute names.

In GitLab by @baayen on Nov 21, 2016, 14:43

Commented on rtctools/data/rtc.pyx line 119

Why not let this method re-raise KeyError?

To let it re-raise the exception, all we need to do is drop the try/except block, and add a ":raises:" to the docstring.

In GitLab by @baayen on Nov 21, 2016, 14:44

Commented on rtctools/data/pi.pyx line 437

Indentation issue?

In GitLab by @baayen on Nov 21, 2016, 14:45

Commented on rtctools/data/pi.pyx line 769

What do we need this for?

In GitLab by @baayen on Nov 21, 2016, 14:45

Commented on rtctools/data/pi.pyx line 743

Alternatively, we could let this check whether "ensemble_size == 1". It would save the bookkeeping of the additional boolean.

In GitLab by @baayen on Nov 21, 2016, 14:47

Commented on rtctools/data/pi.pyx line 588

Is this method private?

In GitLab by @baayen on Nov 21, 2016, 14:48

Commented on rtctools/data/pi.pyx line 606

Rename "ids" to "qualifier_ids" for clarity.

In GitLab by @baayen on Nov 21, 2016, 14:49

Commented on rtctools/data/pi.pyx line 650

I'm a bit confused that we now have "add_series" and "set" methods.

In GitLab by @baayen on Nov 21, 2016, 14:51

Commented on rtctools/data/pi.pyx line 1

We probably don't want this leading space here.

In GitLab by @baayen on Nov 21, 2016, 14:52

Looks good, apart from a few minor things!

In GitLab by @OJMvD on Nov 22, 2016, 09:09

Commented on rtctools/optimization/pi_mixin.pyx line 362

Good question! I did it like this because I copied the method of the csv mixin and changed it around :) But for the pimixin it can actually directly return self._timeseries_import.ensemble_size, as that already is 1 when there is no ensemble.

In GitLab by @OJMvD on Nov 22, 2016, 09:37

Commented on rtctools/data/pi.pyx line 743

It does seem a bit superfluous to do this bookkeeping, but it actually has a function. It can occur that an ensemble has 1 member. Of course, that seems a bit silly and that will never happen structurally.

FEWS does not know beforehand how many ensemble members an ensemble should contain. If for whatever reason not all ensemble member input files of an external forecast are present during the import (some hiccup in the data flow, import ran before transfer to import location was done, etc.), the ensemble size of that external forecast will be the number of ensemble members present. It can occur that that number is 1. FEWS still considers it an ensemble, and will export it as an ensemble as well in a general adapter module (which we use to run RTC-Tools). The import of the general adapter will also expect an ensemble.

If we check whether "ensemble_size == 1" and return False when that's true, the timeseries_export will not have an ensembleMemberIndex and FEWS will not consider it to be an ensemble. That means no data will be imported, and that should be avoided. I see no other way to keep track of this then by checking whether the import file has an ensemble (with n_members from 1 to N), and enforcing that for the export file as well.

In GitLab by @OJMvD on Nov 22, 2016, 10:07

Commented on rtctools/data/rtc.pyx line 119

Try/except stuff is added to pi_mixin.

In GitLab by @OJMvD on Nov 22, 2016, 10:10

Commented on rtctools/data/pi.pyx line 769

Don't really need this, I'll do it differently.

In GitLab by @OJMvD on Nov 22, 2016, 10:16

Commented on rtctools/data/pi.pyx line 588

Should it be?

In GitLab by @OJMvD on Nov 22, 2016, 10:21

Commented on rtctools/data/pi.pyx line 650

Changed it to a "add_header" method.

In GitLab by @OJMvD on Nov 22, 2016, 10:26

Added 1 commit:

646904ef - various tweaks per review

Compare with previous version

In GitLab by @OJMvD on Nov 22, 2016, 10:28

Committed and pushed some tweaks. Two discussion I haven't resolved yet, as you should say something about that ;)

Nosetests do run fine now, I probably had some old file lingering around.

In GitLab by @baayen on Nov 22, 2016, 10:55

Commented on rtctools/data/pi.pyx line 743

So do I get it correctly that FEWS treats ensembles with size 1 fundamentally different from deterministic forecasts? If that is the case, that would indeed suggest we need this additional flag.

In GitLab by @OJMvD on Nov 22, 2016, 10:56

Commented on rtctools/data/pi.pyx line 743

Yes.

In GitLab by @baayen on Nov 22, 2016, 12:39

Commented on rtctools/data/pi.pyx line 588

What would be the use case for it to be called externally?

In GitLab by @OJMvD on Nov 22, 2016, 12:58

Commented on rtctools/data/pi.pyx line 588

Don't know if this is a relevant use case, but someone could decide to make their own post, and then they could use it to build their timeseries_export.xml (instead of building the header themselves).

In GitLab by @OJMvD on Nov 22, 2016, 13:30

Commented on rtctools/data/pi.pyx line 588

Discussed, and we keep this method away from the user (the user also being pi_mixin). Adding headers will be handle by pi.Timeseries.write.

In GitLab by @OJMvD on Nov 22, 2016, 13:30

Resolved all discussions

In GitLab by @OJMvD on Nov 22, 2016, 13:36

Committed and pushed some tweaks. Let me know if more needs to be adjusted.

In GitLab by @OJMvD on Nov 22, 2016, 14:04

Added 1 commit:

2624bb6d - Various tweaks per the review.

Compare with previous version

In GitLab by @baayen on Nov 22, 2016, 14:07

Mentioned in commit cb0d81493d0df1fa60a82639df9aad9fe9d1352b

Deltares / rtc-tools

RTCTOOLS-543: overhaul of PIMixIn - [merged] #1163