Skip to content

add polynomial-decay-helpers#16

Open
florence-bockting wants to merge 16 commits intomainfrom
add_polynomial_decay
Open

add polynomial-decay-helpers#16
florence-bockting wants to merge 16 commits intomainfrom
add_polynomial_decay

Conversation

@florence-bockting
Copy link
Contributor

@florence-bockting florence-bockting commented Apr 2, 2025

Description

  • add polynomial-decay

Checklist

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Changelog item added to changelog/

@florence-bockting florence-bockting marked this pull request as ready for review April 3, 2025 04:41
@florence-bockting
Copy link
Contributor Author

Currently, there is no solution for the potential discontinuity between target and harmonised_spline at the harmonisation_point.

Background:
Depending on the power value the gradient of the polynomial decay drops very fast at the harmonisation point yielding in a 'non-smooth' transition between target and harmonised_spline.

Copy link
Contributor

@znichollscr znichollscr left a comment

Choose a reason for hiding this comment

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

Very nice, minor tweaks. Don't merge yet though please, I want to show you something before you hit the button.

Comment on lines +1 to +12
+ include support for polynomial-decay (besides cosine-decay)
+ we added three new classes/functions to the API in the module `convergence`
+ `PolynomialDecaySplineHelper`
+ `PolynomialDecaySplineHelperDerivative`
+ `get_polynomial_decay_harmonised_spline`
+ whereby the function `get_polynomial_decay_harmonised_spline` is mainly meant as user-interface
This function can be used as value for the `get_harmonise_spline` parameter in `harmonise`.
The function takes the same arguments as `get_cosine_decay_harmonised_spline` and additionally a
`power` argument.
+ The `power` argument can be passed to the function using
`functools.partial(get_polynomial_decay_harmonised_spline, power=2)`
+ An example use-case is added to the `getting-started-tutorial`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+ include support for polynomial-decay (besides cosine-decay)
+ we added three new classes/functions to the API in the module `convergence`
+ `PolynomialDecaySplineHelper`
+ `PolynomialDecaySplineHelperDerivative`
+ `get_polynomial_decay_harmonised_spline`
+ whereby the function `get_polynomial_decay_harmonised_spline` is mainly meant as user-interface
This function can be used as value for the `get_harmonise_spline` parameter in `harmonise`.
The function takes the same arguments as `get_cosine_decay_harmonised_spline` and additionally a
`power` argument.
+ The `power` argument can be passed to the function using
`functools.partial(get_polynomial_decay_harmonised_spline, power=2)`
+ An example use-case is added to the `getting-started-tutorial`.
+ Included support for polynomial-decay (adds to the existing cosine-decay)
+ Added new features to [convergence][gradient_aware_harmonisation.convergence]
+ [PolynomialDecaySplineHelper][gradient_aware_harmonisation.convergence.PolynomialDecaySplineHelper]
+ [PolynomialDecaySplineHelperDerivative][gradient_aware_harmonisation.convergence.PolynomialDecaySplineHelperDerivative]
+ [get_polynomial_decay_harmonised_spline][gradient_aware_harmonisation.convergence.get_polynomial_decay_harmonised_spline]
+ The function [get_polynomial_decay_harmonised_spline][gradient_aware_harmonisation.convergence.get_polynomial_decay_harmonised_spline] is an adapter between the underlying classes and the interface required for the `get_harmonise_spline` parameter in [harmonise][gradient_aware_harmonisation.harmonise].
The function takes the same arguments as [get_cosine_decay_harmonised_spline][gradient_aware_harmonisation.convergence.get_cosine_decay_harmonised_spline] and an additional
`power` argument. The `power` argument can be 'pre-passed' to the function using [functools.partial](https://docs.python.org/3/library/functools.html#functools.partial) e.g. `harmonise(..., get_harmonise_spline=functools.partial(get_polynomial_decay_harmonised_spline, power=2)...)`
+ An example use-case is included in the [getting started tutorial][gradient-aware-harmonisation-getting-started].

Wow super thorough. I am normally much lazier than this when writing changelogs (especially given our current user base is us)

(nwtkia)

The CHANGELOG gets written in past tense. The reason is, when the user reads this, everything will have already been done.

I don't know why, but we use capitals for the start of each dot point.

(hwtatb)

The [][] syntax allows you to do cross-references, which means that readers of the CHANGELOG can click and be taken straight to the module/code that is being discussed.

@@ -0,0 +1 @@
+ fix computation of cosine decay derivative (we forgot a constant value in the formula)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+ fix computation of cosine decay derivative (we forgot a constant value in the formula)
+ Fixed computation of cosine decay derivative (a constant factor was missing)

(nwtkia)

scipy = pytest.importorskip("scipy")


def test_CosineDecaySplineHelperDerivative():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_CosineDecaySplineHelperDerivative():
def test_cosine_decay_spline_helper_derivative():

(nwtkia) For whatever reason, the convention is to use snake_case for function names, even if we're directly referring to a class name

scipy = pytest.importorskip("scipy")


def test_PolynomialDecaySplineHelperDerivative():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_PolynomialDecaySplineHelperDerivative():
def test_polynomial_decay_spline_helper_derivative():

(nwtkia) For whatever reason, the convention is to use snake_case for function names, even if we're directly referring to a class name

@znichollscr znichollscr mentioned this pull request Apr 7, 2025
3 tasks
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.

2 participants