Incorrect handling of <= and >= in event triggers

[dweindl]

In event triggers, < and > are incorrectly interpreted as <= and >= respectively.

This somewhat similar to #2700, but a different issue. Ideally this could be addressed via #2684 and the fixes implemented in #2701.

Reproducer:

import amici
from amici.antimony_import import antimony2amici

antimony2amici("""
a = 1
pp = 0
at time >= a: pp = 1
at time > 2 *a: pp = 2
""", model_name="bla", output_dir="bla")

model_module = amici.import_model_module("bla", "bla")

model = model_module.get_model()

model.setTimepoints([0, 1, 2, 3])

solver = model.getSolver()

rdata = amici.runAmiciSimulation(model, solver)
print(model.getStateIds())
print(rdata.by_id("pp"))
# ->        [0. 1. 2. 2.]
# expected: [0. 1. 1. 2.]

This is not just about numerics. The root functions generated for the two events are the same, despite the different comparison operators:

    root[0] = -a + t;
    root[1] = -2*a + t;

Generated here:

AMICI/python/sdist/amici/sbml_import.py

Lines 3096 to 3109 in a87058e

	# convert relational expressions into trigger functions
	if isinstance(
	trigger,
	sp.core.relational.LessThan | sp.core.relational.StrictLessThan,
	):
	# y < x or y <= x
	return -root
	if isinstance(
	trigger,
	sp.core.relational.GreaterThan
	| sp.core.relational.StrictGreaterThan,
	):
	# y >= x or y > x
	return root

[dweindl]

The current output feels incorrect, but I am not sure how to best address that.

Adding some epsilon to the trigger to distinguish > and >=?
This will cause problems if we have the same trigger, once with > and with >= (root after reinitialization).

Defer event execution until after the output has been written? This feels wrong.

[FFroehlich]

I don't see a good way of differentiating between > and >= in the context of rootfinding, which is why we have the implementation that we have now.

For outputs, one could consider using pre/post event values (depending on sign transition & trigger type) when computing outputs at timepoints that coincide with event timepoints. Sounds like what you meant by

Defer event execution until after the output has been written? This feels wrong.

[dweindl]

I don't see a good way of differentiating between > and >= in the context of rootfinding, which is why we have the implementation that we have now.

Thanks. I was afraid so.

For outputs, one could consider using pre/post event values (depending on sign transition & trigger type) when computing outputs at timepoints that coincide with event timepoints. Sounds like what you meant by

Defer event execution until after the output has been written? This feels wrong.

Yes, that's what I meant. That would "fix" the issue for the the example above. However, I think are other situations where this would cause problems, because events would still potentially be executed while the trigger condition is not exactly true.

[dweindl]

Adding some epsilon to the trigger to distinguish > and >=?

Tried that. This won't fully address the issue.

When adding the smallest representable positive number to the trigger function, the event will not trigger at equality, but it won't necessarily trigger at the next representable timepoint after equality either, due to the tolerances applied in the root finding.

I guess, for now, it's best to keep things the way they are and leave the choice of tolerance to the user, or better, completely avoid situations where the difference between >= and > matters in event triggers.