-
Notifications
You must be signed in to change notification settings - Fork 6
PiMixin: Enable support for ignoring provided forecast dates #1656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option is basically equivalent to preprocessing xml files by removing all forecast_date elements. I would suggest to rename the option as ignore_forecast_date.
My main concern is if this is the correct place to apply preprocessing on xml files. Unfortunately, I don't know if there are alternative ways to achieve this through fews, fews-io, or some other library.
pi_validate_timeseries = True | ||
|
||
#: Read forecast date from timeseries_import | ||
pi_read_forecast_date = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also describe this var in the class docstring.
self.timeseries_export_basename, | ||
binary=self.pi_binary_timeseries, | ||
pi_validate_times=False, | ||
pi_read_forecast_date=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use self.pi_read_forecast_date here as well?
pi_validate_timeseries = True | ||
|
||
#: Read forecast date from timeseries_import | ||
pi_read_forecast_date = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please describe this var in the class docstring.
self.timeseries_export_basename, | ||
binary=self.pi_binary_timeseries, | ||
pi_validate_times=False, | ||
pi_read_forecast_date=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use here self.pi_read_forecast_date as well?
Hi @jackvreeken Could you please review this PR? |
03367be
to
240ff5d
Compare
pi_read_forecast_date=True, | ||
make_new_file=False, | ||
): | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also update the docstring
if el is not None and pi_read_forecast_date: | ||
forecast_datetime = self.__parse_date_time(el) | ||
else: | ||
# the timeseries has no forecastDate, so the forecastDate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mean that you will now just get an error if all start datetimes don't match? That doesn't seem like much of an improvement
An exception is raised if forecast dates were provided and they are not all equal. This option allows users to ignore provided forecast dates.
f59119a
to
08bc156
Compare
|
An exception is raised if forecast dates were provided and they are not all equal. This option allows users to ignore provided forecast dates.