search

pints-team / method-merge-tests

Provides a house for test results for samplers when they are merged into PINTS.
0 stars 0 forks source link

Added gradient descent notebook, to be converted to numerical unit test. #37

Closed MichaelClerx closed 6 months ago

MichaelClerx commented 7 months ago

For these deterministic methods, we can just set up a situation and check that it returns the correct step.

FarmHJ commented 6 months ago

In terms of testing if the method works, I think it is good to go. But will it be good to run the same set of tests as the other optimisation methods to compare their performance, though it might be out of scope with what we want to do in this repo?

While checking this PR, I saw that there is a ‘boundaries’ input for the gradient descent method. If it is ignored in this method, do we want to print a reminder whenever the boundaries are reached?

One really minor comment, do we want to have the naming of the notebooks to have a consistent format? Most of the notebooks seem to use underscores, while the name of this notebook uses dashes.

MichaelClerx commented 6 months ago

Thanks @FarmHJ !

In terms of testing if the method works, I think it is good to go. But will it be good to run the same set of tests as the other optimisation methods to compare their performance, though it might be out of scope with what we want to do in this repo?

I see what you mean, but that would be a different project I think! Here we really only want some kind of alert if the method is accidentally broken / changes its behaviour.

While checking this PR, I saw that there is a ‘boundaries’ input for the gradient descent method. If it is ignored in this method, do we want to print a reminder whenever the boundaries are reached?

Good question! You can raise it as an issue on the PINTS repo if you like, but it will probably just go on the big list of other other minor inconsistencies we haven't sorted out yet. For what it's worth, there is a mention already in the docs. But yeah could easily add a warning too if boundaries are passed in.

One really minor comment, do we want to have the naming of the notebooks to have a consistent format? Most of the notebooks seem to use underscores, while the name of this notebook uses dashes.

Good point. Will fix!

MichaelClerx commented 6 months ago

Have renamed! If it's Ok now, can you use the review button top left to accept?