openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
725 stars 38 forks source link

[REVIEW]: dsmmR: Estimation and Simulation of Drifting Semi-Markov Models #5572

Closed editorialbot closed 3 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@Mavrogiannis-Ioannis<!--end-author-handle-- (Ioannis Mavrogiannis) Repository: https://github.com/Mavrogiannis-Ioannis/dsmmR Branch with paper.md (empty if default branch): Version: v1.0.5 Editor: !--editor-->@Bisaloo<!--end-editor-- Reviewers: @Athene-ai, @RomainAzais, @strevezas Archive: 10.5281/zenodo.13117522

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/55311848a971690d7ba82e421e350ac5"><img src="https://joss.theoj.org/papers/55311848a971690d7ba82e421e350ac5/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/55311848a971690d7ba82e421e350ac5/status.svg)](https://joss.theoj.org/papers/55311848a971690d7ba82e421e350ac5)

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

@Athene-ai & @RomainAzais, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @Bisaloo know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @Athene-ai

📝 Checklist for @RomainAzais

📝 Checklist for @strevezas

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 1 year ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.02 s (864.6 files/s, 308903.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                                8             87           2752           1878
Markdown                         5            232              0            751
TeX                              1             19              0             90
YAML                             2             12              6             56
Rmd                              1             49            121             21
-------------------------------------------------------------------------------
SUM:                            17            399           2879           2796
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 901

editorialbot commented 1 year ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.2202/1544-6115.1326 is OK
- 10.1007/s11009-018-9682-8 is OK

MISSING DOIs

- None

INVALID DOIs

- None
editorialbot commented 1 year ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

Bisaloo commented 1 year ago

:wave: :wave: :wave: @Mavrogiannis-Ioannis @Athene-ai @RomainAzais this is the review thread for the paper. All of our communications will happen here from now on.

As a reviewer, the first step is to create a checklist for your review by entering

@editorialbot generate my checklist

as the top of a new comment in this thread.

These checklists contain the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. The first comment in this thread also contains links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#5572 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. However, I have noted that @RomainAzais is not available to start the review before ~mid-July. We can also use EditorialBot (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@bisaloo) if you have any questions/concerns.

Athene-ai commented 1 year ago

Review checklist for @Athene-ai

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Mavrogiannis-Ioannis commented 1 year ago

Looking forward working with you @Athene-ai @RomainAzais to bring the paper to fruition! Also thank you @Bisaloo for your consistent work and searching. Ioannis

RomainAzais commented 1 year ago

Review checklist for @RomainAzais

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Mavrogiannis-Ioannis commented 1 year ago

Hello and good morning everyone, Yesterday I made a small update to the github page of the package, specifically: adding an Acknowledgements section in the README and DESCRIPTION files, regarding the financing that made the package possible.

It does not affect the paper or the code in any way. In fact, the financing was already mentioned in the paper.md file that you are reviewing. I hope you enjoyed your "vacations"! Hoping to hear from you, soon.

Ioannis

Bisaloo commented 1 year ago

Hi @RomainAzais :wave:, are you able to provide a timeline for when you will be able to complete your review? Thanks a lot for your help!

RomainAzais commented 1 year ago

Hello, Sorry for the delay. I am working on it. I think it will be done for the first half of October. Best, Romain

RomainAzais commented 1 year ago

Hello @Mavrogiannis-Ioannis

I have a few first questions about the package and, more specifically, about model simulation.

If I understand correctly, the package only allows you to simulate model 3 where only the sojourn times drift. Why this restriction?

Beyond that, the output of the simulation function is the sequence of states, without their sojourn times. Don't you think this limits the package's interest in simulation? I also find the documentation a little ambiguous on this point.

Thank you for your feedback on these points.

Mavrogiannis-Ioannis commented 1 year ago

Hello @RomainAzais!

Thank you kindly for your feedback. I will do my best to understand your points and explain my point of view.

a. "If I understand correctly, the package only allows you to simulate model 3 where only the sojourn times drift. Why this restriction?"

The package allows the user to simulate a sequence from any given model. This can be model 1, 2 or 3. This can be seen in the Arguments section of simulate.dsmm(), where obj inherits from all possible objects, namely dsmm and one of: dsmm_fit_nonparametric, dsmm_nonparametric, dsmm_fit_parametric or dsmm_parametric.

Since these objects can have any model (seen in earlier documentation of functions fit_dsmm(), parametric_dsmm() and nonparametric_dsmm()) then we can have any model for the simulate function.

For example, you can run the Examples where you simply switch f_is_drifting and p_is_drifting to obtain Model 1 and 2.

Perhaps your initial concern lies in the Examples section of the function, where we only show an example for the 3rd model.

We chose this way of representation to showcase the functionality of simulate.dsmm(): you can give it any model you previously had (in the example's case obj_model_3) and it's easy to simulate from that point.

b. "Beyond that, the output of the simulation function is the sequence of states, without their sojourn times. Don't you think this limits the package's interest in simulation?...

That is true, the output value is just the "regular" sequence of states. This way the researcher can do as they wish with the sequence. They can obtain the embedded Markov chain and the corresponding sojourn times by running the function base::rle(). From this point of view, it is a simple matter to add to the output of simulate.dsmm(), I can do so at the weekend. Do you consider it necessary?

c. "... I also find the documentation a little ambiguous on this point."

Can you specify exactly which part?

Again, thank you a lot for your feedback. Hope to hear from you soon, Ioannis

RomainAzais commented 1 year ago

Hello @Mavrogiannis-Ioannis

In fact, I hadn't quite understood. Thanks for your reply: it's much clearer now.

Adding to the documentation that base::rle() can be used to obtain the embedded chain and sojourn times is a good idea indeed.

One might also expect simulate.dsmm() to return an object of the S3 class (keeping the parameters used for simulation). I don't know if it's a good idea, but any thoughts on this?

Thank you again for your explanations. Romain

Mavrogiannis-Ioannis commented 1 year ago

Hello @RomainAzais , thank you again for your insightful comments.

It is indeed intriguing for a dsmm_simulate object to exist, inheriting parameters from the object given in the simulate.dsmm() function; this way it will be easier to track down different simulated sequences, making it easy to use for researchers. Added documentation for the base::rle() function would indeed be useful for everyone using the software as well.

I will think about how to formulate this new object (so that it fits in to the other objects in the dsmm "family") and hopefully present it here in the week-end.

Automated tests will also be needed for this new object, similarly to all the other objects in the package. This will take perhaps longer to make, to make sure it's extensive. This will be done so that people can create their own dsmm_simulate object (although it's always easier to use the simulate.dsmm() function).

Thank you again for your feedback! Ioannis

Mavrogiannis-Ioannis commented 1 year ago

So, after a long consideration, I propose the following:

I believe this is a good way to achieve the most clarity, while keeping the objects simple, without repeating too much of the same information across objects and variables. The user should be able to keep track of the models and sequences they are using. They only have to properly name their own variables.

Please, let me know your thoughts on this approach.

Thank you for your time and your patience, Ioannis

Mavrogiannis-Ioannis commented 1 year ago

Hello, the changes as explained on my previous comment are live on github : https://github.com/Mavrogiannis-Ioannis/dsmmR/tree/master

Thank you everyone for your work so far. I'm waiting for more of your comments. Best, Ioannis

RomainAzais commented 1 year ago

Hello @Mavrogiannis-Ioannis

Thanks for the food for thought and the changes. I tested the new version and read the explanations behind it: I think it is clear and easy to use now.

Regarding the package's functionalities, I think it would be worth clarifying that estimation requires only one trajectory observation (in long time) and not several trajectories (both frameworks being allowed in statistics of processes).

On this aspect, the nsim input variable of the simulate.dsmm() function can be confusing (especially with the other optional variable seq_length): maybe prefer names like horizon and max_horizon? (These are really minor points.)

Mavrogiannis-Ioannis commented 1 year ago

Dear @RomainAzais,

First of all, I just discussed with my one of my supervisors during the dsmmR package development, Nicolas Vergne, and we are going to add multiple trajectories as a possibility for the user as well. So I will make this part more well defined when I write the code (in 1-2 weekends...).

As for the simulate.dsmm(), I agree that it requires to spend some time to understand the seq_length attribute etc.

I want to ask you, are names like horizon and max_horizon well defined in the literature? If so, is it possible for you to refer to me an example usage of these terms?

Thank you very much for your support & patience.

Best regards, Ioannis

RomainAzais commented 1 year ago

When simulating a stochastic process (X_t) over a time window [0,T], T is often referred to as the horizon. This may be particularly true in the context of stochastic algorithms or optimal control. See, for example, these lecture notes on Markov decision processes.

max_horizon could denote an upper-bound of the horizon: if I remember correctly, if nsim is greater than seq_length, the simulation stops at seq_length.

By the way, it is great to be able to use multiple trajectories for estimation!

Best, Romain

Mavrogiannis-Ioannis commented 11 months ago

Hello @RomainAzais , thank you again for providing references for your previous comment, it is quite interesting!

After pondering about it, I think the best course of action is as follows, regarding the attributes nsim and seq_length of the simulate.dsmm() function:

I am going to work on the multiple sequences (that I mentioned before), in the next weeks. I'm sorry for taking this long to reply, I had a lot of work for my PhD, a lot of travelling.

Also, I want to thank all of you, @Bisaloo @RomainAzais @Athene-ai for your contribution to this work in the past 6 months. Warm wishes to all of you for the holy season!

Best regards, Ioannis

Bisaloo commented 10 months ago

Hi all, thanks for you work so far and best wishes for this new year!

I'm adding a third reviewer to this review to ensure all relevant aspects of the submission are covered. Thanks!

Bisaloo commented 10 months ago

@editorialbot add @strevezas as reviewer

editorialbot commented 10 months ago

@strevezas added to the reviewers list!

Bisaloo commented 9 months ago

Hi all :wave:, let's try to move this submission forward:

Thanks a lot to everyone for your continued work on this submission. I understand it overlaps with other commitments but in my experience, it's good to try and get it out the door so you can remove it from your mind once and for all and move on :relaxed:.

Mavrogiannis-Ioannis commented 9 months ago

Hello everyone! I tried to implement the multiple trajectories, but it turns out that some theoretical work is still required, so it will be implemented in the future (perhaps during the next year or two).

So, we keep going with what we have right now! Thank you everyone for your work so far -- I am waiting for further comments and suggestions.

Best regards, Ioannis

strevezas commented 9 months ago

Review checklist for @strevezas

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

strevezas commented 9 months ago

Dear @Bisaloo ,

I have completed the revision of the paper. According to the package's github page and after running the relevant examples, almost everything seems to be in order. I have the following remarks:

  1. Concerning the automated tests, already mentioned on the issue opened by you, should I check the relevant box? (https://github.com/Mavrogiannis-Ioannis/dsmmR/issues/2)
  2. A code of conduct should be added, for example using the usethis::use_tidy_coc() function.

Best regards, Samis

Bisaloo commented 7 months ago

Hi @Mavrogiannis-Ioannis :wave:, this submission is almost through. Could you share a timeline for the last few outstanding points please:

Bisaloo commented 6 months ago

Hi @Mavrogiannis-Ioannis, could you please provide a timeline of when you'd be able to address the last minor issues mentioned in my previous comment?

In the absence of a reasonable timeline, our policy is to close the submission and ask to potentially resubmit later, but this means you'd have to restart the process.

Mavrogiannis-Ioannis commented 6 months ago

Dear @Bisaloo, Thank you for your continued support to push this submission forward. I will be able to finish the aforementioned tasks, with a limit at the end of June -- is it a reasonable time-frame or do we have to cut it shorter?

Best regards, Ioannis

Bisaloo commented 6 months ago

Yes, end of June is okay. Let's try and get this out of the door. Thanks!

Bisaloo commented 6 months ago

@editorialbot remind me in 5 weeks

editorialbot commented 6 months ago

Reminder set for @Bisaloo in 5 weeks

crvernon commented 6 months ago

@Bisaloo I am adding the paused tag to this so I don't keep checking in. Feel free to remove it when your reminder expires. Thanks!

Mavrogiannis-Ioannis commented 5 months ago

Good morning everyone,

Some progress has been made. Notably,

Therefore it seems to me that the main questions addressed above are dealt with. Do not hesitate to offer any other questions, ideas or suggestions.

Best regards, Ioannis

editorialbot commented 5 months ago

:wave: @Bisaloo, please take a look at the state of the submission (this is an automated reminder).

crvernon commented 4 months ago

:wave: @Bisaloo could you check back in on this one? Thanks.

Bisaloo commented 4 months ago

Hi :wave: @Mavrogiannis-Ioannis, thanks for the changes, and in particular the code of conduct and the tests. I also confirmed the print() issue I identified has been fixed.

@Athene-ai @RomainAzais @strevezas, could you please have a last look and confirm all seems good to go from your end? As a reminder, here are the links to your respective review comments, where the main issues seemed to be around the presence of tests:

Bisaloo commented 4 months ago

@editorialbot generate pdf

Bisaloo commented 4 months ago

@editorialbot check references

editorialbot commented 4 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.2202/1544-6115.1326 is OK
- 10.1007/s11009-018-9682-8 is OK

MISSING DOIs

- No DOI given, and none found for title: Semi-Markov chains and hidden semi-Markov models t...
- No DOI given, and none found for title: smmR: Semi-Markov Models, Markov Models and Reliab...
- No DOI given, and none found for title: smmR: Simulation, Estimation and Reliability of Se...
- 10.32614/cran.package.drimmr may be a valid DOI for title: drimmR: Estimation, Simulation and Reliability of ...
- No DOI given, and none found for title: INVESTIGATING INDIVIDUAL PATHWAYS OF LEARNING: MOD...
- No DOI given, and none found for title: SemiMarkov: An R Package for Parametric Estimation...
- No DOI given, and none found for title: HMM: Hidden Markov Models
- No DOI given, and none found for title: HiddenMarkov: Hidden Markov Models
- No DOI given, and none found for title: Hidden Semi Markov Models for Multiple Observation...

INVALID DOIs

- None
editorialbot commented 4 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

Bisaloo commented 4 months ago

@Mavrogiannis-Ioannis, I have confirmed myself that the remaining uncheck boxes are now complete. Since they are very practical & objective points (presence of a code of conduct, presence of tests, etc.), I'm confident with moving forward with the acceptance of this submission.

I have proofread the article and submitted a pull request to fix minor formatting issues, and add the missing DOIs: https://github.com/Mavrogiannis-Ioannis/dsmmR/pull/5.

Once the PR is merged, could you please:

I can then move forward with recommending acceptance of the submission.

Mavrogiannis-Ioannis commented 4 months ago

Dear @Bisaloo,

1) The proof-reading is done alongside with my co-authors, and small modifications were made for clarity. You can download it here: https://github.com/Mavrogiannis-Ioannis/dsmmR/actions/runs/10126436083. 2) The tagged release of the software is at: https://github.com/Mavrogiannis-Ioannis/dsmmR/releases/tag/v1.0.5 3) The reviewed software is in Zenodo, through the doi: https://zenodo.org/doi/10.5281/zenodo.13117522

Please, let me know for anything else necessary -- thank you so much for your work so far!

Respectfully, Ioannis

Bisaloo commented 4 months ago

@editorialbot generate pdf

Bisaloo commented 4 months ago

@editorialbot check references

editorialbot commented 4 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/978-0-387-73173-5 is OK
- 10.32614/CRAN.package.smmR is OK
- 10.2202/1544-6115.1326 is OK
- 10.1007/s11009-018-9682-8 is OK
- 10.32614/CRAN.package.drimmR is OK
- 10.18637/jss.v066.i06 is OK
- 10.32614/CRAN.package.HMM is OK
- 10.32614/CRAN.package.HiddenMarkov is OK
- 10.18637/jss.v039.i04 is OK

MISSING DOIs

- No DOI given, and none found for title: smmR: Semi-Markov Models, Markov Models and Reliab...
- No DOI given, and none found for title: INVESTIGATING INDIVIDUAL PATHWAYS OF LEARNING: MOD...

INVALID DOIs

- None
editorialbot commented 4 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left: