Closed MichalisPanayides closed 1 month ago
@MichalisPanayides Hello, Thanks for the detailed review, It was really helpful. The changes have been made. :))
@aadya940 Thanks for addressing all my comments. I can confirm that the contents of all notebooks run fine now. Here's a couple more comments:
benchmarks/
directory I found that the fit
method seems to not satisfy the claims presented in the paper. More precisely when running the benchmarks_fit.ipynb
notebook it seems like for Length = 500
, Length = 1000
, Length = 5000
and Length = 10000
the fit
mehod of chanopy
seems to be slower than the fit_sequence
method of pydtmc. Please let me know if this makes sense and/or if I have misunderstood something. (Picture of the results of Length = 500
running in a local jupyter notebook)Number of Words
/Transition-Matrix size
row and the Mean
/St. dev.
columns. Apologies if I'm missing something there. This comment relates to the following tables:
is_absorbing
Methodsstationary_dist
vs pi
Methodsfit
vs fit_sequence
MethodHello, it was quite strange to know this happened in the fit
function. However, upon careful inspection I realized the caching decorator was missing in the fit
method for some reason which was present earlier while I was writing benchmarks in the paper. Could just be a silly mistake. I assume it will work fine once this PR is merged and am attaching a screenshot below. Really sorry though.
Merged the PR.
Regarding the paper, I see the issue, I'll rewrite the tables a bit. Thanks :)) @MichalisPanayides
Hello, it was quite strange to know this happened in the
fit
function. However, upon careful inspection I realized the caching decorator was missing in thefit
method for some reason which was present earlier while I was writing benchmarks in the paper. Could just be a silly mistake. I assume it will work fine once this PR is merged and am attaching a screenshot below. Really sorry though.
I just checked the changes and it looks like the fit
method is now faster than the fit_sequence
method 👍
Thanks for addressing the comment
@MichalisPanayides Do the tables look better now? https://github.com/aadya940/chainopy/blob/master/JOSS/paper.md
Yes. That looks a lot better now. Will check off the final item in the checklist now 👍
This issue is part of the JOSS review. Here's a list of some comments:
README.md
along with the tests worked fineREADME.md
file. All the suggested changes have been outlined in this PRREADME.md
file it would also be nice to mention the jupyter notebooks inexamples/
andbenchmark/
directoriesseaborn
,yfinance
)How to contribute
sectionThere seem to be some bugs when attempting to run the jupyter notebooks. e.g:
examples/demo.ipynb
the linesim = x.simulate(initial_state="Rainy", n_steps=500)
seems to get the following error (which seems to be fixed if you runsim = x.simulate("Rainy", 500)
instead). I believe there is a bug in theexceptions.py
file:IndexError: tuple index out of range