Skip to content
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

JOSS Paper Review 2 #232

Closed
ziyiyin97 opened this issue Nov 14, 2022 · 5 comments
Closed

JOSS Paper Review 2 #232

ziyiyin97 opened this issue Nov 14, 2022 · 5 comments

Comments

@ziyiyin97
Copy link

My apologies for the late reviews openjournals/joss-reviews#4869

Overall the package shows how to run derivative-free ensemble kalman process in Julia and entails quite clean documentation and examples. There are also a lot of useful references in the manuscript. A few comments:

  1. In general how does this package compare to other Kalman-related packages? like Kalman.jl or other Julia/Python based packages?

Comments for the manuscript:

  1. Line 30 "under certain conditions" -- probably need a reference.

In terms of the code/examples/docs:

  1. The package claims that the prior distribution can be either from Julia Distributions or empirical/sample-based distributions. I would appreciate it if authors could provide an example related to sample-based distributions. (or point to reference)
  2. Also appreciate an example w/ TOML I/O for a few iterations. (or point to reference) I assume these are both related to this module https://github.com/CliMA/EnsembleKalmanProcesses.jl/blob/main/src/UQParameters.jl

I hope these concerns can be easily addressed with more explanations or so.

@odunbar
Copy link
Collaborator

odunbar commented Nov 15, 2022

Hi @ziyiyin97 Thank you very much for your review.

Proposed response:
In paper.md

  • We are currently writing a state-of-field section for the other review, and where our code fits in, we shall include Kalman.jl, GaussianFilter.jl, EnKF.jl in this.
  • "under certain conditions" refers to the standard Kalman assumptions, we shall tighten up this statement.

In the docs

  • We will add a docs for defining a sample-based distribution.
  • We will add a docs tutorial for the TOML interface

In the code

  • We will add a simple example that uses the TOML interface

We shall keep you updated! Thanks again

@ziyiyin97
Copy link
Author

Sounds good!

@odunbar
Copy link
Collaborator

odunbar commented Nov 28, 2022

An update of where we are:

@odunbar
Copy link
Collaborator

odunbar commented Nov 29, 2022

@ziyiyin97 I believe we have now addressed all the issues raised.

@ziyiyin97
Copy link
Author

Thanks for your quick update. The new TOML example in https://github.com/CliMA/EnsembleKalmanProcesses.jl/tree/main/examples/SinusoidInterface needs an updated version of EnsembleKalmanProcess otherwise julia --project will use the previous version of EnsembleKalmanProcess w/o TOMLInterface --i.e. ERROR: UndefVarError: TOMLInterface not defined. But everything else looks good to me and this issue can be fixed by burning a new release. Thanks again for your great work which enriches the Kalman community in Julia!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants