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

Overflow problems in Hill functions #1187

Open
devmotion opened this issue Feb 18, 2025 · 6 comments
Open

Overflow problems in Hill functions #1187

devmotion opened this issue Feb 18, 2025 · 6 comments
Labels

Comments

@devmotion
Copy link
Member

The current implementation of hill etc. is problematic for large arguments.

For instance,

julia> hill(1e40, 1, 1e40, 10)
NaN

I tend to use something like

function myhill(x, v, y, n)
    if x < y
        z = (x / y)^n
        return v * (z / (1 + z))
    else
        return v / (1 + (y / x)^n)
    end
end

which returns

julia> myhill(1e40, 1, 1e40, 10)
0.5

An additional question: What's the reason for symbolically registering these functions instead of traversing them symbolically?

@devmotion devmotion added the bug label Feb 18, 2025
@TorkelE
Copy link
Member

TorkelE commented Feb 18, 2025

Sounds like a good point, never really looked at large inputs. @isaacsas probably have a lot more thoughts when it comes to high-end performance though, and probably have more thoughts than me when it comes to this.

@isaacsas
Copy link
Member

isaacsas commented Feb 18, 2025

I agree that is a more robust implementation. Honestly those definitions are historical, and go back to whoever added them in DiffEqBiological long long ago. Also, when switching to MTK we originally weren't registering, so the expressions were getting substituted, and the current form gives the "standard" ODE terms when Latexified.

I believe the reason we switched to registering them was to avoid them being eagerly substituted into. There has been discussion at times about trying to special case Hill functions in solvers such as JumpProcesses (like we do with mass action reactions), and that would require the ability to know about the presence of such functions in symbolic expressions.

I think @TorkelE added the ability to expand Hill functions to their symbolic expression in one or more places, so we could perhaps offer a system option to eagerly expand them if that would be useful.

If you want to make a PR to update the registered functions to be more numerically robust, with appropriate tests, that would be great and appreciated!

PS - Out of curiosity, why aren't you changing units so that the populations are smaller? In most cases I would tend to adjust units rather than run simulations with such very large populations.

@isaacsas
Copy link
Member

If we do change the definition, and want to allow eager substitution, we'd need to use ifelse right?

@TorkelE
Copy link
Member

TorkelE commented Feb 18, 2025

Depending on how we do change things, we might need to be careful regarding homotopy continuation and structural identifiability analysis which depends on preserving the algebraic expressions. Just something to keep in mind.

@isaacsas
Copy link
Member

isaacsas commented Feb 18, 2025

If we stick to using a registered function that shouldn't be an issue though right? (Since we can just ensure in those contexts we substitute the current formula.)

edit: should -> shouldn't

@TorkelE
Copy link
Member

TorkelE commented Feb 18, 2025

yes, I think that should work out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants