Added JumpStepWrapper

Hi Patrick,
I factored the jump_ts and step_ts out of the PIDController into JumpStepWrapper (I'm not very set on this name, lmk if you have ideas). I also made it behave as we discussed in #483. In particular, the following three rules are maintained:

We always have t1-t0 <= prev_dt (this is checked via eqx.error_if), with inequality only if the step was clipped or if we hit the end of the integration interval (we do not explicitly check for that).
If the step was accepted, then next_dt must be >=prev_dt.
If the step was rejected, then next_dt must be < t1-t0.

We achieve this in a very simple way here: https://github.com/patrick-kidger/diffrax/blob/78b122adf39b2f8d26a79d0ac239a2fb675653a1/diffrax/_step_size_controller/jump_step_wrapper.py#L119-L123
The next step is to add a parameter JumpStepWrapper.revisit_rejected_steps which does what you expect. That will appear in a future commit in this same PR.

I now also added the functionality to revisit rejected steps. In addition, I also imporved the runtime of step_ts and jump_ts, because the controller no longer searches the whole array each time, but keeps an index of where in the array it was previously.
Also I think there was a bug in the PID controller, where it would sometimes reject a step, but have factor>1. To remedy this I modified the following:
https://github.com/patrick-kidger/diffrax/blob/501bed52fb3726aaee41798aaab46d2bdff1a680/diffrax/_step_size_controller/adaptive.py#L569-L574
I think possibly something smaller than just self.safety would make even more sense, I feel like if a step is rejected the next step should be at least 0.5x smaller. But I'm not an expert.
I added a test for revisiting steps and it all seems to work. I also sprinkled in a bunch of eqx.error_if statements to make sure the necessary invariants are always maintained. But this is a bit experimental, so maybe there are some bugs I didn't test for.
I think I commented the code quite well, so hopefully you can easily notice if I made a mistake somewhere.
P.S.: Sorry for bombarding you with PRs. As far as I'm concerned this one is very low priority, I can use the code even if it isn't merged into diffrax proper.

Hi @patrick-kidger,
I got rid of some eqx.error_ifs that I added to my JumpStepWrapper and redid the timing benchmarks. My new implementation was already faster than the old PIDController before, but now this is way more significant, especially when step_ts is long (think >100). Surprisingly, it is faster even when it has to revisit rejected steps. See https://github.com/patrick-kidger/diffrax/blob/345e23ad34abe095d25b8be8d621360afd592165/benchmarks/jump_step_timing.py#L126-L128

Thanks for the review! I made all the edits I could and I left some comments where I need guidance (no hurry though, this is not high priority for me). Also, should I get rid of prev_dt entirely, as you suggested in #483?

Working on the NeuralSDE now and I realised JumpStepWrapper can be useful to wrap a constant step controller as well, for the purpose of making it step to save_ts on top of whichever constant steps it is making. Yes it is more efficient/proper to just do a union of the original constant steps and the save_ts and then feed that into diffrax.StepTo. But there are situations where that could be very inconvenient to do in code, so you can just wrap your constant step controller with a JumpStepWrapper. And I am pretty sure the overhead should be quite minimal (with my implementation the cost per step is no longer O(log n), but just O(1)), so this isn't so bad.
This ties into the SDE interpolation business, where essentially the only good way of interpolating SDEs seems to be just making the solver step to whichever ts you want to save at. I do intend to eventually implement that, but in the meantime this is a useful workaround, I would say.
We don't really have to advertise this use case, but can nevertheless just change the type of JumpStepWrapper.controller from AbstractAdaptiveStepController to AbstractStepController.
What do you think?

Also, should I get rid of prev_dt entirely, as you suggested in #483?

If it's easy to do that in a separate commit afterwards then I would say yes. A separate commit just so it's easy to revert if it turns out we were wrong about something here :D

Thanks for the review, Patrick! I'll probably make the fixes sometime in the coming week. I am also making progress on the ML examples for the Single-seed paper, but it is slower now, due to my internship.

I am very confused about what the correct value of made_jump should be when the step was rejected. By my understanding the original PID controller also got this wrong. However, I'm not sure whether getting this wrong is actually a significant issue, so I want your thoughts on it.
Suppose there is a jump at t=2. I will present 2 possible scenarios, in both of which I think something goes wrong (although maybe diffeqsolve might correct for the issue in scenario B). I wrote them as if JSW and the controller are separate, but the same holds for just the old PID controller.
====== scenario A =======

We start with a step from t0 = 0 to t1 = 1, the controller decides the step will be kept and makes a proposal next_t0 = 1, next_t1=2.5 which gets clipped to next_t1 = 2 and so the JSW writes jump_next_step=True in its state.
We take the next step from t0 = 1 to t1 = 2. Suppose the step is rejected, so next_t0 = 1. But the way the code works now, it will still set made_jump = previous_state.jump_next_step, which has been set to True. But that is wrong because made_jump should reflect whether there is a jump at next_t0 so that the solver knows whether to reevaluate the VF at the start of the next step. Still this is not horrible, since it just causes one extra evaluation of the VF, but the full solution should still be correct. Right?

====== scenario B =======

We start with a step from t0 = 0 to t1 = 1, the controller decides the step will be kept and makes a proposal next_t0 = 1, next_t1=2.5 which gets clipped to next_t1 = 2 and so the JSW writes jump_next_step=True in its state.
We take the next step from t0 = 1 to t1 = 2. Suppose the step is accepted and the controller proposes next_t0 = 2 and next_t1 = 4 the controller returns (correctly) made_jump = previous_state.jump_next_step (=True) and ControllerState(..., jump_next_step=False).
We take the next step from t0 = 2 to t1 = 4. Note that because made_jump was set to True last iteration, the FSAL VF was reevaluated at t = 2. Suppose the step is rejected and the new proposal is next_t0 = 2 and next_t1 = 3. The controller returns  made_jump = previous_state.jump_next_step (=False). This is a problem because there is a jump at next_t0=2, so made_jump should have been True. However that might be fine if the value of the VF which was recomputed at the start of this step is kept for the next step (but I remember thinking that solver state is discarded when the step is rejected, so the recomputed VF also gets discarded).

Another way of seeing this all is through this:

jump_next_step = (there is a jump at next_t1)
previous_state.jump_next_step = (there is a jump at t1)
made_jump should be True iff there is a jump at next_t0

Hence setting made_jump = previous_state.jump_next_step only works when the current step is being accepted, so next_t0 = t1. When rejecting the step, the new made_jump should be equal to the previous step's made_jump which we no longer have access to. The solution would be to add yet another thing to the state, but first I wanted to confirm with you to see if I'm getting this right. What are your thoughts?

So I think the made_jump-on-rejected-step is handled through this line, outside the stepsize controller:
https://github.com/patrick-kidger/diffrax/blob/daec89c8fc9e72e4c2b32def2527a96b80f18b9d/diffrax/_integrate.py#L390
I made the decision to handle some of the step-rejection logic in the main integrate.py loop, on the basis that for those pieces it should be the same for all stepsize controllers.
So I think this fine? Do double-check my logic though! :p
Other than that, one thing I am noticing is that this next_made_jump business is pretty annoying to deal with in the stepsize controller, and I think could probably also be factored out to happen in the main integrate.py loop.

Great, that's exactly the line I was looking for (I must admit I looked in _integrate.py only briefly). But yes this is precisely the piece of the puzzle that my logic was missing, so I think we both independently arrived at the same conclustion that this is probably correct.
Thinking about it now, the next_made_jump situation might be way easier than I thought it was. I'll try to write the code and then I'll write a proof that it does the right thing for any number of stacked JSWs.

Hi Patrick!
I just pushed a new version of this PR, rebased on top of the most current main. I think I addressed everything you asked me to fix.
As it stands this contains 3 commits, contatining:

Everything except 2. and 3.
The changes to at_dtmin and factormax in pid.py
Removing prev_dt from JSW.

I left some conversations unresloved. I did try to fix the things mentioned in those, but I am not sure whether what I did was the best way to tackle that so I wanted to hear your opinion.
Also the test are failing because pyright doesn't know how to import typeguard, which has nothing to do with my changes.


Closing last content item should always trigger a stateChanged event

Current behavior
Depending on content structure, stateChanged event is not fired after closing the last content item.
Expected behavior:
Event should be fired consistently.
Codepen example:

These examples work with localStorage, so you may need to use incognito windows

This codepen from the docs can be used to reproduce the issue. Simply close all three content items and re-run the codepen (or reload the page). Last known state is retrieved from localStorage, but it still includes the last content item.
I've created another codepen with just one content item to exemplify correct behavior. You can close the only content item and it will be gone after reloading.

If we make sure AbstractContentItem always emits stateChanged after its child is removed, everything seems to work as expected.
diff --git a/src/js/items/AbstractContentItem.js b/src/js/items/AbstractContentItem.js
index 386b48a..57784e9 100644
--- a/webapp/tmp
+++ b/webapp/tmp
@@ -132,6 +132,8 @@ lm.utils.copy( lm.items.AbstractContentItem.prototype, {
                 */
                } else if( !(this instanceof lm.items.Root) && this.config.isClosable === true ) {
                        this.parent.removeChild( this );
+               } else {
+                       this.emitBubblingEvent( 'stateChanged' );
                }
        },

I'm not sure this is the right approach, though.
The problem seems related to how components are rearranged on user interaction. For example:

action
content

Create 2 items
row > stack

Drag right
row > column > 2 stacks

Close right
row > stack

If we instead drag it to the bottom:

action
content

Create 2 items
row > stack

Drag bottom
row > 2 stacks

Close bottom
stack

If we then close the last item, stateChanged is emitted in the second scenario but not in the first one.

Because the itemDestroyed events are fired in that case, an easy workaround would be to detect empty layout contents on the itemDestroyed events.
myLayout.on('itemDestroyed', function(){
  setTimeout(function(){
    if (myLayout.toConfig().content.length === 0){
      // the last item was removed
      // persist myLayout.toConfig now to localStorage
    }
  }, 100); // delay because layout config gets updated some ticks later
});

I left out the debouncing of the itemDestroyed events and the logic to avoid a double detection when the stateChanged is fired as it should.


Separate handling of release candidates

As I understand it, we basically have two options for the "default" root version of the docs: master (or some other branch of choice) or the latest (or highest by semver ordering) tag.
This logic doesn't quite work when the latest tag is a pre-release and master is development.  For example, I have a project where master is the development version, tag 0.4.3 is the latest stable release, and 0.5.0rc0 is the most recent tag.  (I think this is not an uncommon scenario.)  In this case, it would be nice to have an option to build docs for release candidates/pre-releases, but keep the default as the highest non-rc tag.
Does this seem reasonable, and would it be difficult to pull off?
(Unrelated: thanks for building this package. It's awesome!)

Thanks for the unrelated note :)
This makes sense, I can't believe I didn't realize it sooner since we do something similar at work. It's been a while since I worked on this so I'm not sure how long it'll take me to write a (hopefully) modular solution (maybe let users override the logic by specifying a function in conf.py or something).
It'll probably take a weekend of work by my estimates. I'll try to work on it in a couple of weeks.


Changed Button to Link, and removed unneccessary props

Fixes #...
https://github.com/wagtail/wagtail/issues/11581
Fixes the above issue.
Renamed Button to Link
Removed the aria-haspopup and dialogTrigger
Updated tests

The test in which there is no href tag raises the following error:
45:13  error  The href attribute is required for an anchor to be keyboard accessible. Provide a valid, navigable address as the href value. If you cannot provide an href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md

Not sure what to do to fix it.

@advikkabra which file has the Eslint issue?
In the CI I can only see one failure, from a unit test.