search

peilun-he / PDSim

Web application for the polynomial diffuison model
Apache License 2.0
0 stars 1 forks source link

Needs better usage documentation #4

Open bkrayfield opened 10 months ago

bkrayfield commented 10 months ago

After reviewing the repository, I want to encourage an expansion of the usage documentation. I was able to look over what is provided both in the package and the Shiny app. The Shiny app is really straight forward, but it depends too much on the previous knowledge of the previous literature. To someone in Finance, it will make sense, but outside, it will be more difficult.

For example,

Based on the reviewer checklist, the package must have usage documentation that is accessible to outside reviewers.

taqtiqa-mark commented 10 months ago

I'll second this point. My initial observation was that the project would benefit to pick one scope and stick to it. Either this is a general utility or it targets a finance use case. If the authors elect to make the interface generic there may be use for a tutorial document that shows how to configure settings for the financial application at hand. And vice versa.

peilun-he commented 9 months ago

The "How to Use PDSim (GUI)" section gives a brief explanation of all parameters and is accompanied by screenshots of the application. Users can understand the meaning of parameters from this section.

taqtiqa-mark commented 9 months ago

Users can understand the meaning of parameters from this section.

My observation is that this depends on the user background (target audience/assumed knowledge), and at different places the user background assumed/required changes. So I believe this claim is contradicted by my experience.

peilun-he commented 9 months ago

Hi @taqtiqa-mark, I added some further explanations of parameters in the "How to Use PDSim (GUI)" section. Hopefully this will make them more clear to all users. Currently, this application is designed for users who want to simulate commodity futures data based on two models, so we have to assume they have some knowledge in this area. However, we have also exported some major functions (KF, EKF, UKF, etc. ). They are some very general estimation algorithms, and can be applied to other areas easily. The usage of these functions are given in "How to Use PDSim (R Script)" section.

bkrayfield commented 8 months ago

I am happy with the changes that have been made. If @taqtiqa-mark is happy I would like to close the comment.

taqtiqa-mark commented 8 months ago

I'll complete my review tomorrow.

peilun-he commented 4 months ago

This line of comment is now finished, could we please close the thread, unless you have a different opinion. Thank you.

bkrayfield commented 4 months ago

@taqtiqa-mark Are you all good with this now?

taqtiqa-mark commented 4 months ago

Not really. There appears to be some confusion over what they are calculating (I've repeatedly asked to have Figure 1 from the original paper reproduced and they are steadfast in refusing to do that, not sure why)

Another issue seems to have arisen related to the potential presence of nuisance parameters (where for some parameter values other parameters 'vanish') but I need to look more closely into this. If present this definitely needs to be called out in the user document but also guarded/warned about in the UI they provide. But as I said this isn't entirely clear yet.

peilun-he commented 3 months ago

We thank both referees for their continued consideration. We intend to create a suite of unit tests as originally requested as well as containerization. We believe this will resolve all remaining issues. Of course, we will respond in detail once we complete these unit tests and the containorisation and aim to get this to both reviewers by end of April.

taqtiqa-mark commented 3 months ago

Please note the request to replicate the SS Figure 1 and Figure 4 still stand. You should then also be able to provide such figures for the FP model.