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

Fast and safe Siegert implementation #1811

Closed
AlexVanMeegen opened this issue Oct 22, 2020 · 11 comments · Fixed by #1849
Closed

Fast and safe Siegert implementation #1811

AlexVanMeegen opened this issue Oct 22, 2020 · 11 comments · Fixed by #1849
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects

Comments

@AlexVanMeegen
Copy link
Contributor

AlexVanMeegen commented Oct 22, 2020

Is your feature request related to a problem? Please describe.
The siegert_neuron requires a numerically tricky integration where an exp(s**2) and and 1 + erf(s) interact in the integrand. Currently, this problem is solved using a substitution that leads to an infinite integral which is better behaved but still tricky. The problem and the currently implemented solution is documented here.

Describe alternatives you've considered
Instead of the above substitution, one can use the scaled complementary error function to boil down the integrals to a single, well-behaved one. Here are the details: siegert_numerics.pdf. In all my tests so far this works pretty well.

Additional context
This idea is approve by @mhelias; I also asked @tomtetzlaff for his opinion but his answer is pending. I can provide a fully Python based implementation in case someone wants to test it.

@stinebuu stinebuu added this to To do in Models via automation Oct 28, 2020
@stinebuu stinebuu added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Oct 28, 2020
@clinssen
Copy link
Contributor

clinssen commented Nov 9, 2020

@AlexVanMeegen: this is a very in-depth description, and I'm not sure I understand the implications without further discussing this in detail. Could we ask you to take care of filing a pull request for this, if you (and @tomtetzlaff) indeed decide that this confers sufficient advantages?

@AlexVanMeegen
Copy link
Contributor Author

@clinssen, I am not sure I see why a full PR is needed to understand the implications. Could you be a bit more specific what you are missing? Then I can try to answer before creating a full blown PR.

Nonetheless, I am of course most willing to make the PR if desired!

@clinssen
Copy link
Contributor

I don't understand the domain well enough, to see the complete implications. If you are proposing to implement a mathematical re-formulation that yields the same results, but more accurately or with more numerical stability, this would I think be very welcome! And could you also say how it might impact runtime?

@AlexVanMeegen
Copy link
Contributor Author

AlexVanMeegen commented Nov 10, 2020

Yes, precisely: This is all about the numerical evaluation of a tricky integral (Eq. 2 in my notes). My proposal is simply to use a different mathematical reformulation of the integral than the one that is currently used. The new reformulation is, to my best current knowledge, better behaved.

Naively, I think this will also improve runtime without affecting the accuracy, but this of course needs a careful benchmark.

@AlexVanMeegen
Copy link
Contributor Author

AlexVanMeegen commented Nov 11, 2020

@tomtetzlaff also approves this, so I guess it is indeed sensible. I'll prepare a PR, please bug me in case I forget about this!

@clinssen
Copy link
Contributor

Great, thank you! Could you please also check whether there is a unit test covering the behaviour of the neuron near the point of numerical instability?

@AlexVanMeegen
Copy link
Contributor Author

Sure, I'll try to keep this in mind!

@jougs
Copy link
Contributor

jougs commented Dec 1, 2020

Shouldn't this issue have the word "safe" in the title, rather than "secure"? The same applies for #1849. Thanks!

@AlexVanMeegen
Copy link
Contributor Author

Agreed, my bad!

@AlexVanMeegen AlexVanMeegen changed the title Fast and secure Siegert implementation Fast and safe Siegert implementation Dec 1, 2020
@jougs
Copy link
Contributor

jougs commented Dec 1, 2020

If it were a really aggressive optimization, I'd of course also have accepted furious 😈

@AlexVanMeegen
Copy link
Contributor Author

"These optimizations were performed by trained professionals, don't try this at home."

@clinssen clinssen linked a pull request Dec 15, 2020 that will close this issue
@clinssen clinssen moved this from To do to Done in Models Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Models
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants