Skip to content

fix: respect maxevent when maxtime is finite - #49

Open
sdwfrost wants to merge 2 commits into
AlgebraicJulia:mainfrom
epirecipes:pr/fix-review-maxevent-main
Open

sdwfrost wants to merge 2 commits into
AlgebraicJulia:mainfrom
epirecipes:pr/fix-review-maxevent-main

Conversation

@sdwfrost

@sdwfrost sdwfrost commented Mar 30, 2026

Copy link
Copy Markdown

Problem

run! accepted both maxevent and maxtime, but previously a finite maxtime silently disabled the caller-supplied maxevent by replacing it with typemax(Int).

That meant callers asking for "stop after at most N events, but no later than time T" were only getting the time bound. It also left the API with an arbitrary hidden default event cap (100) when no stopping criterion was supplied.

Fix

This PR now does two things:

  1. maxevent defaults to typemax(Int) rather than an arbitrary finite cap.
  2. run! now errors if the caller leaves both bounds unbounded, i.e. maxevent == typemax(Int) and maxtime == Inf.

With that change, finite maxevent and finite maxtime compose cleanly, and calls that would otherwise run without any stopping criterion now fail with a clear message asking the caller to specify at least one bound.

Testing

  • kept the regression test showing that maxevent=2 and maxtime=10.0 stop after exactly two events
  • added a regression test that run!(...) without any finite bound throws a helpful error
  • updated affected tests/examples on this branch to pass an explicit bound where needed

@kris-brown kris-brown left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, this is definitely an improvement!

I think I originally chose this weird secret behavior of maxevent getting ignored when a maxtime is specified because the default maxevent is quite arbitrary (100). And I picked a finite, arbitrary value because I wanted it to be possible to call run! without any user supplied keyword arguments.

But now I see that the cleanest thing is to have maxevent default to typemax(Inf) just like maxtime defaults to Inf!

Comment thread src/ABMs.jl
@@ -486,7 +486,6 @@ end

function run!(abm::ABM, rt::RuntimeABM, output::Traj;
save=_->nothing, maxevent=MAXEVENT, maxtime=Inf, dt=0.1)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose the default for maxevent should be typemax(Int) and the default for maxtime can be Inf. There should be an error in the first line if neither has been specified by the caller.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Note that, once this is done, any tests which do not supply maxevent or maxtime will be broken, and they would have to be changed (e.g. to add the kwarg maxevent=100) in order to work again.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sdwfrost

Copy link
Copy Markdown
Author

Updated this PR to match the review direction: maxevent now defaults to typemax(Int), run! errors if both bounds are left unbounded, and I added a regression test for the no-bound error plus explicit bounds at affected branch-local call sites.

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.

2 participants