Skip to content

Conversation

@SarahAlidoost
Copy link
Collaborator

closes #66
closes #68

🔴 After merging #61, I will update the docs.

@SarahAlidoost SarahAlidoost marked this pull request as ready for review December 17, 2025 15:26
@SarahAlidoost SarahAlidoost changed the title Split leaf and root dynamics Split leaf and root dynamics optimization notebooks Dec 17, 2025
@SarahAlidoost
Copy link
Collaborator Author

@michielkallenberg @ronvree @SCiarella @fnattino in this pull request, STE method is used for the sigmoid approximation for the parameter SPAN in leaf_dynamics, see the issue #68. Please let me know what you think about this approach, or any other ideas/suggestions. Thanks!

@sonarqubecloud
Copy link

Copy link
Collaborator

@SCiarella SCiarella left a comment

Choose a reason for hiding this comment

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

Thanks @SarahAlidoost, I really prefer this new approach to the sigmoid because the gradient now is much more controlled.

Later, we could even allow the user to set a lower sharpness to have a better gradient for parameter optimization, but right now the value of 1000 that you have chosen looks ok.

I approve this PR for merge 👍

@ronvree
Copy link

ronvree commented Dec 18, 2025

I also think this is a really good idea! During the development it's much better to remove the ambiguity of whether some sigmoid approximates the original model well enough. Great suggestion!

Although the soft threshold might in some case have a more realistic biophysical interpretation (as also mentioned by Francesco in issue #68), I feel these discussions should be separated from the initial model development.

If I understand correctly the sharpness should ideally still be set based on the magnitude of the input if we want to guarantee there are no issues during optimization?

Would it make sense to implement the thresholds as nn.Module instances so it's easier to inspect their behavior and see the impact of any potential adjustments?

Thanks @SarahAlidoost! I also approve the PR for merge

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.

sigmoid approximation and Straight-Through Estimator: SPAN in leaf_dynamics [Task] split and rerun optimization nb for leaf and root dynamics.

4 participants