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

Remarks about paper - episode 2 [JuliaCon Proceedings review] #402

Closed
gdalle opened this issue Feb 11, 2024 · 5 comments
Closed

Remarks about paper - episode 2 [JuliaCon Proceedings review] #402

gdalle opened this issue Feb 11, 2024 · 5 comments

Comments

@gdalle
Copy link
Contributor

gdalle commented Feb 11, 2024

JuliaCon/proceedings-review#133

I have re-read the paper, overall the presentation has improved indeed! In particular, this time around I have finally understood the syncing aspect which makes your algorithm different. Thus I was able to focus a bit more on the mathematical part at the beginning.

My main remaining issues are:

  • The introduction of Coevolve still doesn't make it clear what is yours and what is theirs. I suggest introducing a new name, maybe CoevolveSync, since from what I understand this is your major contribution to an existing algorithm
  • The section on inverse methods is rather confusing and could be shortened
  • In Algorithm 5 it would be nice to be able to compare with a naive queueing algorithm, to see the benefits of syncing
  • On the easiest benchmark I don't understand why queueing methods perform poorly: are these instances 1-dimensional (in the mark space)?
  • The comparison with Tick is great, I would also add brief references to other point process libraries in Python and R

You can find all my remarks as annotations on the PDF file. Is it okay as a format for exchanging remarks? Can you read them?

@gzagatti
Copy link
Contributor

@gdalle thanks for your additional comments. I will work on them and respond to your remarks next week. The annotations on the PDF file work great for me.

gzagatti added a commit to gzagatti/JumpProcesses.jl that referenced this issue Mar 4, 2024
@gzagatti
Copy link
Contributor

gzagatti commented Mar 4, 2024

@gdalle Thanks again for your time to review our paper. Please find below my comments to the issues you raised.

It would also be great that you keep an eye at PR #404, where we'll finalize our revision. It would be easier if this time we could incorporate all your comments before merging the PR, such that when the paper goes to whedon it can be completed without yet another round of revisions and PR.

JuliaCon/proceedings-review#133

I have re-read the paper, overall the presentation has improved indeed! In particular, this time around I have finally understood the syncing aspect which makes your algorithm different. Thus I was able to focus a bit more on the mathematical part at the beginning.

That's great!

My main remaining issues are:

* The introduction of `Coevolve` still doesn't make it clear what is yours and what is theirs. I suggest introducing a new name, maybe CoevolveSync, since from what I understand this is your major contribution to an existing algorithm

Initially, my plan was to implement Coevolve. Once I realized that it was possible to improve Coevolve to run in sync with ODE solvers, I implemented the improved version which I coincidentally named CoevolveSynced. However, during the PR process at the time, we came to the conclusion that it was redundant to have both Coevolve and CoevolveSynced in the same library because the latter supersedes the former. From a maintenance perspective, this strategy requires less effort and is less error-prone. In the PR, I also provide some evidence that the improved Coevolve is a bit faster (and this was before we made improvements to the PR and the library in general).

At this point, it would be confusing for a user of the library to have the algorithm referenced as CoevolveSynced in the paper and as Coevolve in the library. Therefore, it would be better to emphasize and make it clear in the paper that our version of Coevolve improves the original one. I tried to use Coevolve in the paper only when referencing the original algorithm or the aggregator in the library.

The idea of running the original Ogata algorithm can be traced back even earlier than the COEVOLVE paper (Farajtbar et al 2017). The next reaction method from Gibson and Bruck (2000) shares some similarities with COEVOLVE. Furthermore, Daley and Vere-Jones (2003) already discussed the idea of synced algorithms after listing Ogata's modified thinning algorithm (Algorithm 7.5.IV). In page 272, they say (with the main challenge highlighted by me):

"Algorithm 7.5.IV can be extended to cover the situation where the evolution of the conditional hazard function depends on additional random processes, themselves evolving jointly with the given point process. The immediate requirements are for the existence of explicit algorithms for calculating the intensity and for finding local bounds L(·) and M (·) that take into account current and past values of the auxiliary variables. A deeper difficulty, however, relates to the need to simulate forward not only the point process but also the auxiliary variables on which it depends. For auxiliary variables that change only slowly, this may not be a serious handicap, but for longer-term predictions, a full model is needed from which the point process and auxiliary variables can be jointly simulated."

However, until our paper, we have not seen an algorithm implementation of such idea, which is where our main contribution lies.

I have modified a few parts of the introduction to reflect the above discussion.

* The section on inverse methods is rather confusing and could be shortened

The reason that the section on inverse method is longer is because it derives CHV simple and CHV full. These derivations are not directly available in the sources we mentioned, but are important for implementing the TPP simulations which we benchmark in the experiment section. It also sets the stage for the theoretical discussion that follows that section. Nevertheless, I tried to improve on the points you annotated. I hope that it makes the section easier to follow.

* In Algorithm 5 it would be nice to be able to compare with a naive queueing algorithm, to see the benefits of syncing

The naive "queuing" algorithms are the NRM and Tick. The benchmark section serves to compare the different implementation of queuing algorithms.

* On the easiest benchmark I don't understand why queueing methods perform poorly: are these instances 1-dimensional (in the mark space)?

The easiest benchmark are not unmarked, but they are constant rate processes. That means a lot of the tricks that were developed for thinning this type of processes make them faster than queueing. In the future, it would be a good idea to port these ideas to queueing algorithms.

I added a sentence to the paper on that.

* The comparison with Tick is great, I would also add brief references to other point process libraries in Python and R

Done.

You can find all my remarks as annotations on the PDF file. Is it okay as a format for exchanging remarks? Can you read them?

Below you find my comments on some of the annotations that I could not directly address in the paper (page and line numbers refer to the manuscript you annotated):

  • page 1, line 5: Maintained "also provides". It is short and conveys the equivalent idea that the library offers the advertised capability.

  • page 2, line 121: $T = - \ln(V) / \lambda \sim \exp(1)$ is correct. See this Wikipedia Section for a derivation.

  • page 2, line 156: Replace $t(0)$ with $\tilde{t}_{n-1}$. Here $0$ refers to the interval $\Delta \tilde{t}$. I have re-written the derivative with $\Delta \tilde{t}$ to make it more clear.

  • page 3, line 162: Remove reference to $\varphi$. It is hard to remove the notation for the flow because it plays an important role in the derivation of CHV full. Alternatively, I have expressed Equation 4.8 in terms of $\varphi$. I hope it makes the connection more clear.

  • page 3, line 179 (and other similar comments): Equation 4.2 is indeed the correct reference. I have added an extra equivalence in the equation in an attempt to make the problem more obvious.

  • page 5, line 343: It would require extra effort to show the difference as colored lines in the listed algorithms. It is also not just a matter of highlighting a few lines, but the algorithms have slight different steps. We try to highlight the difference in our discussion in the main text.

  • page 9, line 724: Indeed CHV (full or simple) was not implemented in JumpProcesses.jl. This is mentioned previously around line 670. Implementation of CHV in JumpProcesses.jl is current work in progress (see issue CHV / True Jump method implementation discussion #291 and CHV branch).

isaacsas added a commit that referenced this issue Mar 4, 2024
@gzagatti
Copy link
Contributor

gzagatti commented Mar 4, 2024

PR #404 was merged to the paper branch. You can find the latest draft in JuliaCon/proceedings-review#133.

@gdalle
Copy link
Contributor Author

gdalle commented Mar 5, 2024

Thanks, the changes look good, in particular the clarifications about Coevolve! Here are a few typos I spotted but otherwise I'm okay with this version
Zagatti et al. - 2023 - Extending JumpProcesses.jl for fast point process .pdf

@gzagatti
Copy link
Contributor

gzagatti commented Mar 5, 2024

That's great news!

I fixed the typos in PR #406. Once it gets merged we can close this issue.

@gdalle gdalle closed this as completed Mar 5, 2024
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