search

JuliaCon / proceedings-review

6 stars 1 forks source link

[REVIEW]: StateSpaceModels.jl: a Julia Package for Time-Series Analysis in a State-Space Framework #28

Closed whedon closed 8 months ago

whedon commented 4 years ago

Submitting author: !--author-handle-->@raphaelsaavedra<!--end-author-handle-- (Raphael Saavedra) Repository: https://github.com/LAMPSPUC/StateSpaceModels.jl/ Branch with paper.md (empty if default branch): Version: Editor: !--editor-->@ranjanan<!--end-editor-- Reviewers: @dehann, @mschauer Archive: Pending

Status

status

Status badge code:

HTML: <a href="https://submissions.juliacon.org/papers/bb5fbbdb5e7888d4d980239719fc98bd"><img src="https://submissions.juliacon.org/papers/bb5fbbdb5e7888d4d980239719fc98bd/status.svg"></a>
Markdown: [![status](https://submissions.juliacon.org/papers/bb5fbbdb5e7888d4d980239719fc98bd/status.svg)](https://submissions.juliacon.org/papers/bb5fbbdb5e7888d4d980239719fc98bd)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@dehann & @mschauer, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/JuliaCon/proceedings-reviews/invitations

The reviewer guidelines are available here: https://proceedings.juliacon.org/guide/reviewers. Any questions/concerns please let @matbesancon know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @dehann

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

Review checklist for @mschauer

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Paper format

Content

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @dehann, @mschauer it looks like you're currently assigned to review this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/JuliaCon/proceedings-review) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/JuliaCon/proceedings-review:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

matbesancon commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Attempting to check references...
whedon commented 4 years ago 

OK DOIs

- 10.18637/jss.v078.i10 is OK

MISSING DOIs

- https://doi.org/10.1137/141000671 may be missing for title: Julia: A fresh approach to numerical computing
- https://doi.org/10.1093/acprof:oso/9780199641178.001.0001 may be missing for title: Time Series Analysis by State Space Methods
- https://doi.org/10.1109/proc.1965.4237 may be missing for title: Linear System Theory: The State Space Approach
- https://doi.org/10.1109/9780470544334.ch9 may be missing for title: A new approach to linear filtering and prediction problems
- https://doi.org/10.21105/joss.00615 may be missing for title: Optim: A mathematical optimization package for Julia
- https://doi.org/10.17771/pucrio.irr.33804 may be missing for title: Simulating Low and High-Frequency Energy Demand Scenarios in a Unified Framework–Part I: Low-Frequency Simulation

INVALID DOIs

- None
matbesancon commented 4 years ago

@raphaelsaavedra check the references to update above

raphaelsaavedra commented 4 years ago

@matbesancon Thanks! I have included the suggested DOIs, except for "A new approach to linear filtering and prediction problems". I included what I believe is the correct DOI, 10.1115/1.3662552. The updated version is already in the master, do I need to make a new release?

matbesancon commented 4 years ago

No I don't think it is necessary now

On Tue, Jul 9, 2019, 21:10 Raphael Saavedra notifications@github.com wrote:

@matbesancon https://github.com/matbesancon Thanks! I have included the suggested DOIs, except for "A new approach to linear filtering and prediction problems". I included what I believe is the correct DOI, 10.1115/1.3662552. The updated version is already in the master, do I need to make a new release?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuliaCon/proceedings-review/issues/28?email_source=notifications&email_token=AB2FDMSFSCQYA62ZKASFDBDP6TPAPA5CNFSM4H7CQDAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZRHKEA#issuecomment-509768976, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2FDMWVXQ44LJDEWYH62WDP6TPAPANCNFSM4H7CQDAA .

matbesancon commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Attempting to check references...
whedon commented 4 years ago 

OK DOIs

- 10.1137/141000671 is OK
- 10.1093/acprof:oso/9780199641178.001.0001 is OK
- 10.1109/proc.1965.4237 is OK
- 10.18637/jss.v078.i10 is OK
- 10.1115/1.3662552 is OK
- 10.21105/joss.00615 is OK
- 10.17771/PUCRio.IRR.33804 is OK

MISSING DOIs

- None

INVALID DOIs

- None
raphaelsaavedra commented 4 years ago

@matbesancon are we allowed to upload a preprint of the juliacon paper on arxiv using a different template?

matbesancon commented 4 years ago

yes

matbesancon commented 4 years ago

ping @dehann, @mschauer to continue the review

mschauer commented 4 years ago

Nice to meet you. So I should start stating that I share some interest in this topic, see https://github.com/mschauer/Kalman.jl This should be manageable according to PNAS criteria. Actually, my package is somewhat opinionated and small, so you would expect a different package to be the go-to package for this topic.

mschauer commented 4 years ago

@whedon commands

whedon commented 4 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references
mschauer commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

raphaelsaavedra commented 4 years ago

@mschauer thanks for taking your time! I should warn you that we have just released a significant performance update in v0.2.3. From what I've seen in the checklist, you have already installed the package, so you might want to update it :)

matbesancon commented 4 years ago

ping @dehann, @mschauer to continue the review :)

mschauer commented 4 years ago

Thanks for keeping it up, this is up next.

mschauer commented 4 years ago

Dear authors,

I have some feedback regarding the design. Some of your implementation choices appear not very idiomatic to me, but you might have good reasons for them. On the other hand, this might be helpful feedback otherwise.

Let's start looking at 

struct StateSpaceModel
    y::Matrix{Float64} # observations
    Z::Array{Float64, 3} # observation matrix
    T::Matrix{Float64} # state matrix
    R::Matrix{Float64} # state error matrix
    dim::StateSpaceDimensions
    missing_observations::Vector{Int}
    mode::String
end

The first thing to notice is that you restrict your users basically throughout to Float64 matrices of type Array. What about embedded systems with Float32 accuracy? What if their filtering problems are large (and can only be handeled with say banded or block-diagonal matrices? What about static arrays (might be helpful in realtime applications?) At some places you seem to have it right,

function big_update_K!(K::AbstractMatrix{Typ}, P_Ztransp_invF::AbstractVector{Typ}, T::AbstractMatrix{Typ}, t::Int) where Typ <: AbstractFloat
    @inbounds @views K[:, t] = T * P_Ztransp_invF
    return
end

tolerates a range of different inputs. This flexibility in input (container and value-) types is also important for interaction with other packages, e.g. if you want to replace maximum-likelihood estimation by an MCMC method.

Another issue: Does your package support the online application of the filtering algorithm? The Kalman filter is a filter, after all... You mention the focus on time-series applications, but in the end, all the implementations, even the square root filter, are there (you wrote them), it would be nice to make them available in a more generic fashion.

raphaelsaavedra commented 4 years ago

Hello @mschauer, thank you for taking your time to do this review.

Regarding the generality of the StateSpaceModel structure, we believe you are absolutely right. The package would certainly benefit from allowing a wider range of types. In fact, we already do this inside the code, as you pointed out. We will try to address this point as soon as possible, as it seems like a simple change, at least in terms of allowing other Float types.

With respect to your enquiry about the online application: this is indeed something that we do not allow at the moment. We do have a function called kfas that runs the filtering and smoothing steps, but we should definitely allow users to call only the filter if they so desire. We will make filtering available so that online applications can become a possibility. This should also be a simple change.

Once again, thank you for your valuable feedback. We will make the changes and comment here once they are done.

raphaelsaavedra commented 4 years ago

Hello @mschauer,

@guilhermebodin and I have discussed and addressed some of your concerns. We have modified the package to allow any float accuracy within the model definition and the filters (https://github.com/LAMPSPUC/StateSpaceModels.jl/pull/101).

With respect to the online application of the filter, after some discussion we have decided not to implement it in the current form of the package. Within our package, the filter data (state information, covariances, Kalman gain etc.) are kept in memory and saved in their full form (all time periods). We do this to allow for studies that can be pertinent within the context of time series analysis. In order to make an online implementation of the filter, we would have to create new filter functions and structures from scratch. While we believe there is definitely value in doing that, we think this might be outside of the current scope of our package, which is mainly focused in time series. This might end up motivating a new package that can deal with online filtering applications.

As an unrelated note, we have also implemented statistical diagnostics to evaluate the model specification and estimation (https://github.com/LAMPSPUC/StateSpaceModels.jl/pull/100).

Thank you for the feedback and do not hesitate to comment again if you have further observations.

mschauer commented 4 years ago

Hi, this is a good change. Can you relax to Real in struct SquareRootFilter{T <: AbstractFloat} as AbstractFloat is to narrow, e.g. dual numbers are only Real? Then you can also remove all <: AbstractFloat in function definitions as

function univariate_smoother(model::StateSpaceModel{Typ}, kfilter::UnivariateKalmanFilter{Typ}) where Typ <: AbstractFloat

it is enough to contain in one place (the struct itself). I am looking at the estimation next.

raphaelsaavedra commented 4 years ago

We agree this makes sense and it has been changed in https://github.com/LAMPSPUC/StateSpaceModels.jl/pull/107.

raphaelsaavedra commented 4 years ago

Hi @matbesancon, I hope you don't mind if I bump this thread since there hasn't been any progress since October.

One of the reviewers hasn't shown up after 8 months, how should we proceed? Is it possible to find another reviewer?

dehann commented 4 years ago

HI Sorry, i have the review just forgot to upload. My bad sorry. Will do it as soon as possible

dehann commented 4 years ago

Dear Authors,

I liked the paper as well as the aim of the package. I understand the package as standardizing linearized state-space models for JuliaLang, and found the structure familiar from existing literature. I did have a little trouble switching to the nomenclature in the paper, but that is not serious.

Specific comments:

Great, and sorry again for not uploading sooner!

raphaelsaavedra commented 4 years ago

Hi @dehann, thank you for your feedback.

Please find below my answers to your comments:

Your other suggestions that weren't explicitly addressed above were implemented in the paper.

Furthermore, please note that since the initial pdf was generated, we have added a new section regarding basic statistical diagnostics.

We hope you're satisfied with the new version. If you require any further changes, do not hesitate to ask 🙂

raphaelsaavedra commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

raphaelsaavedra commented 4 years ago

PS: please remember to tick the boxes in the first post according to your impressions :)

dehann commented 4 years ago

Done thanks, one more suggestion is to add links to the demo data in the Docs too (already in Readme).

raphaelsaavedra commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

raphaelsaavedra commented 4 years ago

Thanks for the suggestion. We have added a reference pointing to the repo, [14], in the Applications section, where we mention that the code and data for examples can be found in the examples folder.

matbesancon commented 4 years ago

Hi @mschauer, we had some delays on the JuliaCon proceedings handling, would you be ok with continuing this review?

vchuravy commented 3 years ago

@whedon commands

whedon commented 3 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Re-invite a reviewer (if they can't update checklists)
@whedon re-invite @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# All commands can be run on a non-default branch, to do this pass a custom 
# branch name by following the command with `from branch custom-branch-name`.
# For example:

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks

# Ask Whedon to do a dry run of accepting the paper and depositing with Crossref
@whedon accept

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository

EiC TASKS

# Invite an editor to edit a submission (sending them an email)
@whedon invite @editor as editor

# Reject a paper
@whedon reject

# Withdraw a paper
@whedon withdraw

# Ask Whedon to actually accept the paper and deposit with Crossref
@whedon accept deposit=true
vchuravy commented 3 years ago

@whedon remind @mschauer in 5 days

whedon commented 3 years ago

Reminder set for @mschauer in 5 days

whedon commented 3 years ago

:wave: @mschauer, please update us on how your review is going.

raphaelsaavedra commented 3 years ago

Any updates here? I see the proceedings are already out (https://proceedings.juliacon.org/).

matbesancon commented 3 years ago

Hi @raphaelsaavedra, sorry for the delays we experienced

matbesancon commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

PDF failed to compile for issue #28 with the following error:

Can't find any papers to compile :-(