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

Update JumpProcesses documentation #372

Merged
merged 25 commits into from
Dec 14, 2023
Merged

Update JumpProcesses documentation #372

merged 25 commits into from
Dec 14, 2023

Conversation

gzagatti
Copy link
Contributor

@gzagatti gzagatti commented Nov 21, 2023

Following @gdalle review of our JuliaCon paper (thank you very much!), I have modified the documentation to address all his concerns.

The goal was to make the documentation more accessible to beginners and those coming from point process theory. I have added two new sections "Getting Started with JumpProcesses in Julia" and "Temporal Point Processes (TPP) with JumpProcesses".

Please see my comments to all the points raised by the reviewer.

From Issue #365.

Home page

* I think a few sentences might be warranted about what a jump / point process is and to whom it is useful ("statement of need" section of the review). I know people landing on this page will probably know already, but it doesn't cost much to make it accessible for people who don't.

I added a paragraph to situate a new user. I have updated the list of links and split it into two lists, one for tutorials and one for references and APIs.

The front page also guides the user to a new page "Getting Started with JumpProcesses in Julia" to align with the DifferentialEquations documentation which also contains a similar page.

I have also aligned the sidecar with the DifferentialEquations documentation to ensure that a user familiar with that library documentation will also be familiar with JumpProcesses documentation.

Simple Poisson Processes in JumpProcesses

* I don't understand the specific roles of the aggregator and the stepper

I have rephrased the paragraph where we introduce the aggregator to provide more intuition on this type of object. Also, see "Getting Started with JumpProcesses in Julia" for more accessible discussion on aggregators and steppers.

* For the process with varying birth rate, you state that "death does not change `u[1]`", which seems weird to me. Isn't it rather birth that doesn't change `u[2]`?

This paragraph was indeed very confusing. I have re-written it to make it more clear. The main point is that birth and death processes are orthogonal to the state variable u. The death process does not affect the birth process, because the later only obeys seasonal fluctuation. In other words, it is not because a lot of people are dying that agents will change their fertility behavior.

Continuous-Time Jump Processes and Gillespie Methods

* Is there a way to implement the variable rate case of time-dependent infection rate without a global variable `H`? Same question for other models on the page

I have removed the dependency on the global variable. H is now part of the parameters p that are used to initialize the problem.

* When it comes to controlling saving behavior, what exactly is saved by the callback mechanism? During the first read it was unclear to me that the saving referred to the solution values.

I have added some notes to clarify that we are only saving the state variable u at end of section "Reducing Memory Use: Controlling Saving Behavior"

Piecewise Deterministic Markov Processes and Jump Diffusion Equations

* The jump diffusion equation may be a bit unclear for those who (like me) don't know SDEs. I assume dW is a Brownian motion?

I have added a beginner-level discussion below the equation.

* Why does the SDE version look so different from the PDMPs on the last plot?

This is just a coincidence, since all paths are stochastic they might actually look very different from simulation to simulation. In any case, I have added a discussion below the plots to explain what the different problem definitions bring to the simulated paths.

I have also reduced the complexity of the problems by removing the copied jump. It doesn't really add much to the overall goal of the tutorial and might be confusing. The main goal of the tutorial is to show how JumpProcesses can be used with discrete problems, ODEs, or SDEs.

Spatial SSAs with JumpProcesses.jl

(nothing)

Mathematical Specification of a problem with jumps

* I would put the blurb at the beginning much earlier in the docs, perhaps even before the tutorials

I have added a "Getting Started with JumpProcesses in Julia" to align with the DifferentialEquations documentation which also contains a similar page. A concise tutorial in the same style as the parent library should make JumpProcesses more accessible.

* An example of `DiscreteProblem` definition would be welcome before stating that `JumpProblem` must wrap it

That is now discussed in "Getting Started with JumpProcesses".

Jump solvers

* The distinct roles of the aggregator and stepper are still unclear to me

The distinction should be made clear and accessible in "Getting Started with JumpProcesses".

Small typos

* [Fix typos in docs #366](https://github.com/SciML/JumpProcesses.jl/pull/366)

Thanks!

From Issue #370.

Functionalities

* The main reason I wrote [PointProcesses.jl](https://github.com/gdalle/PointProcesses.jl) was to work with processes that generate arbitrary marks (not just multivariate counting processes). I think it is possible to do that with JumpProcesses.jl but I'd like some pointers

I have added a complete new tutorial on temporal point processes (TPP) with JumpProcesses. It guides someone familiar with TPP theory on how to use JumpProcesses to simulate TPPs.

As an added bonus, I am using PointProcess to define SciMLPointProcess which inherits AbstractPointProcess whose API contained many TPP constructs.

The tutorial builds SciMLPointProcess while apply it to a concrete case the marked Hawkes process.

* Other interesting aspects of my library include loglikelihood computation and fitting. I think fitting is not in your scope, but is there any chance you would consider implementing a loglikelihood computation given a process and a sample?

I have implemented all the API from PointProcess except for fitting.

Regarding TPP parameter fitting, we cannot always derive an analytical estimator via maximum likelihood for TPPs. Therefore, it is beyond the scope of the tutorial to implement such method. I have added a discussion of this fact at the end of the tutorial.

Fix issues #365, #370 and JuliaCon/proceedings-review#133.

@isaacsas
Copy link
Member

@gzagatti thanks! I will give this a look through so we can hopefully get it merged very soon. (Hopefully tonight or tomorrow.)

docs/make.jl Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Nov 21, 2023

Pull Request Test Coverage Report for Build 7152592399

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 7 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.3%) to 89.692%

Files with Coverage Reduction New Missed Lines %
src/aggregators/prioritytable.jl 1 86.86%
src/extended_jump_array.jl 1 81.58%
src/coupling.jl 2 78.18%
src/spatial/spatial_massaction_jump.jl 3 85.45%
Totals Coverage Status
Change from base Build 7091794717: -0.3%
Covered Lines: 2297
Relevant Lines: 2561

💛 - Coveralls

Copy link
Member

@isaacsas isaacsas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate your effort with the getting started tutorial, I don't think it improves the new user experience overall. I'd prefer if the getting started tutorial just points users to the appropriate tutorial based on their background (e.g. point-processes, jump processes in physics or chemistry, etc). Otherwise it needs to explain the equations it is using, and explain what is appearing in them for each of these different communities so they can understand what it is doing.

Perhaps we should instead add a brief but fully explained mathematical discussion as requested in the review as the "getting started tutorial", and then link there to the concrete tutorials for people with different backgrounds. That would avoid needing to try to write a single tutorial that is accessible to all fields, while providing a single point that sends users to the tutorial that might work best for them.

docs/Project.toml Show resolved Hide resolved
docs/src/assets/Project.toml Show resolved Hide resolved
docs/src/getting_started.md Outdated Show resolved Hide resolved
docs/src/getting_started.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
Comment on lines 592 to 593
length(integrator.p[5]) > 0 &&
deleteat!(integrator.p[5], rand(1:length(integrator.p[5])))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An array doesn't seem like the right data structure to use if you want to support dynamic, random removal during a simulation (unless the length of the array is always quite small). It is fine for the tutorial, but I'd be careful using this in benchmarks.

Copy link
Contributor Author

@gzagatti gzagatti Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced the array with a dictionary. As I understand, dictionaries in Julia are implemented as Hash maps so insertions and deletions should be O(1).

We don't use random removal on the MultivariateHawkes benchmark. The brute-force version of the benchmark only pushes to the array.

docs/src/tutorials/jump_diffusion.md Outdated Show resolved Hide resolved
docs/src/tutorials/jump_diffusion.md Outdated Show resolved Hide resolved

```@example tut3
jump_prob = JumpProblem(prob, Direct(), jump, jump2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why drop this? Without it the tutorial only shows examples that use one jump, which may give a misleading impression that one can't have more than one jump?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with making the second jump different in some way, but I'd prefer we keep multiple jumps in the examples to make clear that is possible.

Copy link
Contributor Author

@gzagatti gzagatti Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped because I felt the goal of this section (as the title suggests) was to illustrate variable jump in the context of SDEs. Therefore, adding another jump in this section can be confusing to the reader. My first reaction was "Why not just double the rate to achieve the same effect?" It got me thinking that there was some sort of trick that I was not getting.

I think if the intention is to demonstrate the case of multivariate jumps in SDEs. Why don't we just create another section appropriately titled to illustrate such problems? Then we can just mix the variable and constant jumps from the previous two sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a suggestion with this extra section.

docs/src/tutorials/simple_poisson_process.md Outdated Show resolved Hide resolved
docs/src/tutorials/simple_poisson_process.md Outdated Show resolved Hide resolved
docs/src/tutorials/simple_poisson_process.md Outdated Show resolved Hide resolved
docs/src/tutorials/simple_poisson_process.md Outdated Show resolved Hide resolved
docs/src/tutorials/simple_poisson_process.md Outdated Show resolved Hide resolved
@isaacsas
Copy link
Member

@gzagatti left you some comments.

@gzagatti
Copy link
Contributor Author

Thanks @isaacsas.

Perhaps we should instead add a brief but fully explained mathematical discussion as requested in the review as the "getting started tutorial", and then link there to the concrete tutorials for people with different backgrounds. That would avoid needing to try to write a single tutorial that is accessible to all fields, while providing a single point that sends users to the tutorial that might work best for them.

Just to make sure I understand this part, before I start refactoring the "Getting Started" page. We want to keep this new page, but it needs some refactoring. Is that correct?

Next, are you suggesting that we just keep a high-level discussion and point to relevant tutorials (less work, we can keep some high-level equations) or that we include a full mathematical description for different audiences and then point to relevant tutorials (more work, since I feel I would be re-writing a lot of the equations that are already present in other tutorials).

@isaacsas
Copy link
Member

I was suggesting not having any code on the "getting started" page, but instead having it direct users to the appropriate starting tutorials for their background (Poisson Processes, Stochastic Chem Kinetics, or your new Point Processes tutorial).

The review comment suggested adding the math right up front, so we could add such a paragraph in the index page or the getting started page, showing jump-diffusion equations mathematically, briefly explaining the terms, and then mentioning they include as special cases jump processes (temporal point processes), and PDMPs.

@gzagatti
Copy link
Contributor Author

@isaacsas I have addressed most of your feedback and thanks for taking the time.

Regarding the "Getting Started", I got a bit stuck. I made a few changes to keep it simpler. But I think removing all the code will sort of make it redundant with the index since the index already contains some brief explanation of the maths and links to all tutorials.

My goal with "Getting Started" was to go through the motion of setting the problem up and solving with little else. It even breaks it down on the same 3 steps as in DifferentialEquations and discuss the very basic constructs in the library. For instance, the reviewer was confused with terms like aggregator and stepper. This page provides brief explanation of them in context.

I feel that a beginner would get a bit lost with the remaining tutorials because they are much longer and address more specific models.

Feel free to remove the "Getting Started" tutorial if you still not convinced. Or even to refactor it to your liking.

@isaacsas isaacsas mentioned this pull request Dec 6, 2023
@isaacsas isaacsas merged commit 25c3fcf into SciML:master Dec 14, 2023
5 of 8 checks passed
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

Successfully merging this pull request may close these issues.

None yet

3 participants