Add optional raw spec parameter for x13_arima_analysis() - #9550
Conversation
| spec += _make_forecast_options(forecast_periods) | ||
| spec += "x11{ save=(d11 d12 d13) }" | ||
| spec += "x11{ save=(d11 d12 d13) \n savelog=(fd8 m7 q)}" | ||
|
|
There was a problem hiding this comment.
It happens below, but if the user passes a raw spec you should trap that error seperatrly and let them know there is a problem with their spec.
There was a problem hiding this comment.
I'm not clear what you mean here. Are you saying it should parse or otherwise validate the raw spec file before sending it to x13? Or that there should be a separate path for spec file errors?
There was a problem hiding this comment.
Lower in the file there is an exception handelign block that raises if somethign bad happens. You shoudl seperate the cases where statsmodels wrote the spec file from where the user directly passed the spec, so that you can tell the user that something is wrong with their spec.
There was a problem hiding this comment.
Thanks for clarifying. Rather than adding separate handling logic, I added in an extra warning when the x13 error occurs with a user-provided spec file. The error returned by the existing _check_errors() function does a good job of showing where the errors are line-by-line already, so it didn't seem worth it to add a separate error check.
| freq : str | ||
| Must be givein if ``endog`` does not have date information in its | ||
| index. Anything accepted by pandas.DatetimeIndex for the freq value. | ||
| rawspec : str or Path |
There was a problem hiding this comment.
Looking at this, why not allow the user to pass additional options beyone what is known. It looks like this can be of type dict[str, Union[dict[str, ??], str]]. This would just add these to the statsmodels generated spec.
There was a problem hiding this comment.
That could definitely be useful for adding miscellaneous parameters. Since rawspec expects something that will essentially replace the spec file it might make more sense to add a second parameter called "addspec" or something, to make it clearer that it's not replacing the spec file outright.
ChadFulton
left a comment
There was a problem hiding this comment.
Looks pretty good to me, thanks @ropeladder! I have a few questions / comments.
| # if specfile string (or path) is passed | ||
| else: | ||
|
|
||
| if ((not None) in [diff, exog, start, freq]): # or (not outlier) or trading: |
There was a problem hiding this comment.
I think the intention here is to raise an error if any of those arguments are set, but doesn't this just test if the value True is in that list?
There was a problem hiding this comment.
Oops! Should work now.
|
|
||
| rawspec_text = None | ||
|
|
||
| if os.path.exists(rawspec): |
There was a problem hiding this comment.
Technically it might be more idiomatic here to do a try/except block like:
try:
with open(rawspec) as f:
rawspec_text = f.read()
except FileNotFoundError:
if "{" in rawspec:
rawspec_text = rawspec
else:
raise ValueError("rawspec argument provided but not valid path"
" or spec string")There was a problem hiding this comment.
open() doesn't like super long filenames, so I used except (OSError, FileNotFoundError):.
| # (d11=final seasonally adjusted series) | ||
| # (d12=final trend cycle) | ||
| # (d13=final irregular component) | ||
| if spec_outputs: |
There was a problem hiding this comment.
What happens in the case that the user doesn't have any outputs already defined?
There was a problem hiding this comment.
I added more splicing to deal with other scenarios.
| be treated as a valid spec file. Other parameters for the spec file | ||
| will be IGNORED. | ||
| Series data and required output formats are spliced into spec file | ||
| before it is passed to x12/x13. |
There was a problem hiding this comment.
I think this probably needs a little more explanation for how the user is expected to set up their spec file. e.g.:
- You must have a
series{}block that does not containdataorfilearguments. - You must have a
save{}block.
There was a problem hiding this comment.
Added some details.
|
LGTM. Do you have anything else planned, or are you happy with it now? @bashtage is there anything else you'd like to see here before it is merged? Thanks again, great addition! |
That should be it, thanks! |
|
Thanks @ropeladder! |
This commit adds an option to pass in a raw
.specfile to thex13_arima_analysis()function, either as a path or as a string.We needed a way to use more advanced configuration options for x13, but adding all the specifications to the statsmodels function itself seemed like an exercise in futility. (the manual for x13 is nearly 300 pages) The string substitution is hopefully not too brittle.