Skip to content

Conversation

@simon-hirsch
Copy link
Owner

No description provided.

@simon-hirsch simon-hirsch requested review from BerriJ and Copilot May 7, 2025 08:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces two new scaled sigmoid link functions and a generic inverse link to facilitate function inversion.

  • Added ScaledInverseSigmoidLink and ScaledSigmoidLink in sigmoidlinks.py with methods for the link, its inverse, and corresponding derivatives.
  • Introduced a GenericInverseLink in generic.py to swap the link functions and derivatives.
  • Updated init.py files to expose the new link functions and the GenericInverseLink.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/rolch/link/sigmoidlinks.py Added implementations for two scaled sigmoid links with their respective methods.
src/rolch/link/generic.py Added a GenericInverseLink class to invert provided link functions.
src/rolch/link/init.py Updated module exports to include the new sigmoid links and generic inverse link.
src/rolch/init.py Updated package exports to include the new link functions.
Comments suppressed due to low confidence (2)

src/rolch/link/sigmoidlinks.py:38

  • In ScaledInverseSigmoidLink.inverse_derivative, the multiplication by (self.upper - self.lower) and subsequent division by (self.upper - self.lower) cancel each other out. Simplify the expression to 'sigmoid(x) * (1 - sigmoid(x))' for clarity.
* (1 - sigmoid(x)) / (self.upper - self.lower)

src/rolch/link/sigmoidlinks.py:80

  • In ScaledSigmoidLink.link_derivative, the factor (self.upper - self.lower) is multiplied then divided by the same term, resulting in redundancy. Consider simplifying the derivative to eliminate the unnecessary operations.
* (1 - sigmoid(x)) / (self.upper - self.lower)


Args:
link_function (LinkFunction): The Link function to invert
link_support (Tuple): The support of the link function
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is misleading, we want to pass the support of the inverted link function here, right?

Suggested change
link_support (Tuple): The support of the link function
link_support (Tuple): The support of the inverted link function

simon-hirsch and others added 3 commits May 7, 2025 12:13
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@BerriJ
Copy link
Collaborator

BerriJ commented May 8, 2025

This is a great Idea! However, before merging this I'd like to make the following changes:

  • Rename generic.py to generic_inverse.py as the "generic" link function is defined in rolch/base/link. This way we make it more explicit that this only contains a generic inverse link.
  • Adjust implementation of InverseSoftPlusLink this is a natural candidate to inherit from the new class, so we need an implementation of the non inverted SoftPlusLink
  • Update docs
  • Think about special cases such as links with subclasses: SqrtLink > SqrtShiftTwoLink > SqrtShiftValueLink

For now, we should move this into the draft stage IMHO.

@BerriJ BerriJ added links enhancement New feature or request labels May 8, 2025
@BerriJ BerriJ added this to the First stable Version milestone May 8, 2025
@BerriJ BerriJ marked this pull request as draft June 3, 2025 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request links

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants