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

NAN in multivariant stochastic SEIR Model #1009

Open
2 tasks done
nijawa opened this issue Apr 19, 2024 · 14 comments
Open
2 tasks done

NAN in multivariant stochastic SEIR Model #1009

nijawa opened this issue Apr 19, 2024 · 14 comments
Assignees
Labels
class::discussion Used for discussions on software and model . loc::backend This issue concerns the C++ backend implementation. model::sde This issue concerns any kind of stochastic differential equation-based model.

Comments

@nijawa
Copy link
Contributor

nijawa commented Apr 19, 2024

Bug description

NAN in compartments in SEIR2V Model

Version

Windows

To reproduce

  1. build code
  2. execute sde_seir2v_example
    (https://github.com/SciCompMod/memilio/blob/1009-nan-in-multivariant-seir-model/cpp/examples/sde_seir2v.cpp)
    seir_bug

Relevant log output

No response

Add any relevant information, e.g. used compiler, screenshots.

No response

Checklist

  • Attached labels, especially loc:: or model:: labels.
  • Linked to project
@nijawa nijawa added the class::bug Bugs found in the software label Apr 19, 2024
@mknaranja mknaranja added the model::sde This issue concerns any kind of stochastic differential equation-based model. label Apr 19, 2024
@mknaranja mknaranja moved this from Product Backlog to 🏗 Development in MEmilio: Equation-Based-Models Development Apr 19, 2024
@mknaranja mknaranja added the loc::backend This issue concerns the C++ backend implementation. label Apr 19, 2024
@nijawa
Copy link
Contributor Author

nijawa commented Apr 19, 2024

I use std::min and std::max instead of std::clamp to ensure that flows are in the correct bonds. Usage of std::clamp gives a "invalid bounds arguments passed to std::clamp" error which is consistent with the NAN output

@mknaranja
Copy link
Member

mknaranja commented Apr 19, 2024

@nijawa : Yes, nan is probably an incorrect input but clamp should prevent any appearance of nan beforehand. So there's potentially another problem? My questions still is "How does the nan appear?" Can you do a break point before the first nan and check what the actual miscomputation is?

@nijawa
Copy link
Contributor Author

nijawa commented Apr 19, 2024

@mknaranja At t = 21.5 we have the compartment InfectedV1V2 at 0.014175747933863577 with an ingoing flow of 0 and an outgoing flow of 0.14175747933863578. With step size = 0.1 that would be 0.014175747933863577 - 0.014175747933863578 in the next step which would be negative. It is displayed as -0.0000 but with higher precision the value seems to be -1.73472347597681e-18. A negative compartment will of course result in nan.

I suspect that the issue is that we dont minimize the flow to the compartment but rather we minimize it to compartment / step_size. compartment / step_size * step_size can differ from compartment in floating point calc.

I dont have a good way to resolve this right now

@nijawa
Copy link
Contributor Author

nijawa commented Apr 19, 2024

Clamping/maximizing with compartment / step_size - eps might work

@mknaranja
Copy link
Member

mknaranja commented Apr 19, 2024

I think that compartment / step_size * step_size can differ from compartment is not the problem, that basically is the thing for all flops. The problem, as you state, rather is that we do not have a tolerance like eps here. Can you make a suggestion based on @reneSchm's already suggested changes? You can now branch from main.

@mknaranja mknaranja changed the title NAN in multivariant SEIR Model NAN in multivariant stochastic SEIR Model Apr 19, 2024
@nijawa
Copy link
Contributor Author

nijawa commented Apr 19, 2024

my seir2v model is not yet in main yet so if I branch from main that model will be missing I think? There also still is a bit I have to tidy up before it should get merged into main.

Is there a prefered eps value for memilio? Else I would propose 1e-6

@mknaranja
Copy link
Member

Yes, the change would just address the functionality not merge the model yet.

As we are in double precision, eps (or I propose tol) should be much smaller. Minimum 1e-10 or rather 1e-14.

@nijawa
Copy link
Contributor Author

nijawa commented Apr 19, 2024

Clamping with a tolerance also is problematic if compartments are 0. To fix this I could either use a multiplicative tolerance (something like 1-1e-10) or use std::max(std::min to enforce non-negative flows over clamp errors

@reneSchm
Copy link
Member

I suggest clamping to zero exactly, and only use tolerances on the upper bound. Then you don't need a relative tolerance, at least I would've just subtracted tol. You can test which works better for you.

You should avoid getting NaNs in the first place, min/max will probably not reliably recover the results if there are NaNs involved.

One other thing you could consider is post-processing the results of get_flows in the advance function. There you can just take the max of each compartment and 0. If you've clamped the flows before, the error from that should be at most around 1e-16.

@reneSchm
Copy link
Member

my seir2v model is not yet in main yet so if I branch from main that model will be missing I think?

Depends. If you have nothing commited, then you can just pull again to update all unrelated files. If there are edits that would be overwritten, git will complain.
If you did make commits to your branch, you can try rebasing on main, or if that fails merge it.
Then there is also the good old method of opening a new branch and copying files manually. Can be easier, occasionally.

@reneSchm reneSchm added class::discussion Used for discussions on software and model . and removed class::bug Bugs found in the software labels Apr 19, 2024
@nijawa
Copy link
Contributor Author

nijawa commented Apr 19, 2024

I suggest clamping to zero exactly, and only use tolerances on the upper bound. Then you don't need a relative tolerance, at least I would've just subtracted tol. You can test which works better for you.

You should avoid getting NaNs in the first place, min/max will probably not reliably recover the results if there are NaNs involved.

One other thing you could consider is post-processing the results of get_flows in the advance function. There you can just take the max of each compartment and 0. If you've clamped the flows before, the error from that should be at most around 1e-16.

The tolerance works on the upper limit of the clamp. std::clamp(flow, 0, compartment / step_size - tol) doesnt seem to work if compartment = 0

@nijawa nijawa closed this as completed Apr 19, 2024
@github-project-automation github-project-automation bot moved this from 🏗 Development to ✅ Done (Sprint) in MEmilio: Equation-Based-Models Development Apr 19, 2024
@mknaranja mknaranja reopened this Apr 20, 2024
@github-project-automation github-project-automation bot moved this from ✅ Done (Sprint) to Sprint Backlog in MEmilio: Equation-Based-Models Development Apr 20, 2024
@mknaranja mknaranja moved this from Sprint Backlog to 👀 Review in MEmilio: Equation-Based-Models Development Apr 20, 2024
@nijawa nijawa linked a pull request Apr 20, 2024 that will close this issue
12 tasks
@jubicker
Copy link
Member

@nijawa @reneSchm what's the status here?

@reneSchm
Copy link
Member

I am not sure. If the problem still persists, it would be possible to catch these errors in the simulation, as the SDE models use their own simulations anyways.

@mknaranja
Copy link
Member

mknaranja commented Nov 28, 2024

@nijawa Could you provide an example if this is still an issue or if it has been resolved? And is this the same issue as #1101 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
class::discussion Used for discussions on software and model . loc::backend This issue concerns the C++ backend implementation. model::sde This issue concerns any kind of stochastic differential equation-based model.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants