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

Added switch_dim parameter to ChangePoints kernel #1671

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

clwgg
Copy link

@clwgg clwgg commented Apr 19, 2021

PR type: bugfix / enhancement

Related issue(s)/PRs: Resolves #1195, also see https://stackoverflow.com/questions/59613675

Summary

Proposed changes
I propose to add a switch_dim parameter to the ChangePoints kernel to make it usable on multidimensional inputs. This parameter allows to define ChangePoints on one particular dimension of the input space, while leaving all dimensions accessible to the kernels being switched. This makes it possible to switch kernels on any (combination of) input dimensions based on one switching dimension.

As an example, this setup allows structures such as:

def switched_k():
    return SquaredExponential(active_dims=[0]) * SquaredExponential(active_dims=[1])

k = ChangePoints([switched_k(), switched_k()], switch_dim=1, location=init_loc)

This example has 2D input, and the kernel being switched is a product of kernels on the 1st and 2nd dimension. The switch_dim parameter allows to define a ChangePoint for this entire kernel product based on the 2nd dimension of the input.

What alternatives have you considered?
I also have a working version of making the active_dims parameter accessible to the ChangePoints kernel as proposed on the SO link above. The issue with this was, that it only allows to switch kernels on the dimension on which the ChangePoint is defined, e.g.:

k = SquaredExponential(active_dims=[0]) * ChangePoints([SquaredExponential(), SquaredExponential()], active_dims[1], location=init_loc)

Fully backwards compatible: I think so, but I will be adding tests to make sure

State of this PR

The PR is still a bit rough, but I'll be adding tests in the coming days if there's general interest in merging this ChangePoint setup. Alternatively, I'm also happy to contribute the active_dims version of handling multiple input dimensions I mention above.

PR checklist

  • New features: code is well-documented
    • detailed docstrings (API documentation)
    • notebook examples (usage demonstration)
  • The bug case / new feature is covered by unit tests
  • Code has type annotations
  • Build checks
    • I ran the black+isort formatter (make format)
    • I locally tested that the tests pass (make check-all)
  • Release management
    • RELEASE.md updated with entry for this change
    • New contributors: I've added myself to CONTRIBUTORS.md

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #1671 (aa677b5) into develop (3e4b9f1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1671   +/-   ##
========================================
  Coverage    97.00%   97.00%           
========================================
  Files           91       91           
  Lines         4168     4170    +2     
========================================
+ Hits          4043     4045    +2     
  Misses         125      125           
Impacted Files Coverage Δ
gpflow/kernels/changepoints.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e4b9f1...aa677b5. Read the comment docs.

@st--
Copy link
Member

st-- commented Apr 20, 2021

Hi @clwgg, thanks for thinking this through & contributing the PR:)
Your switch_dim suggestion seems much more sensible than using the inner kernels' active_dims, as that may not even be well-defined. I'd be happy to merge this in once you've added the tests & updated the RELEASE.md and CONTRIBUTORS.md :) You may also want to post/update the answer on Stackoverflow once this is merged in!

@clwgg
Copy link
Author

clwgg commented Apr 26, 2021

While looking into covering this use-case with tests, I discovered that there are ChangePoint-related tests implemented both in tests/gpflow/kernels/test_kernels.py as well as a single one in tests/gpflow/kernels/test_changepoints.py. Is there any preference on where to implement the new tests? Also, should the ChangePoint tests be unified in a single file (either test_kernels or test_changepoints)? If so, I'd be happy to contribute that either here or in a separate PR.

@st--
Copy link
Member

st-- commented Jul 6, 2021

@clwgg apologies for dropping the ball on this. I hope you'd still be interested to finish off this PR!?
Regarding test locations, I would suggest adding tests for this to test_changepoints.py; if you think it would be best to unify all ChangePoint tests in one file, feel free to move the other from test_kernels.py as well, but not required for merging this PR : )

This parameter allows to define the ChangePoint on one particular dimension of
the input space. All other dimensions are still available to the kernels being
switched, and can be selected via active_dims.
@clwgg
Copy link
Author

clwgg commented Jul 27, 2021

I hope you'd still be interested to finish off this PR!?

Yes, absolutely! Sorry, it took me a moment to get back to it. I've added a few commits on the changepoint tests. These first reorganize all changepoint-related tests into the same file, and then add a few new ones. I think the new tests cover the behavior newly introduced here, and seem to be in line with the types of things the previous changepoint tests test. Please let me know what you think, and if you can think of something else changepoint-related that should be tested.

Also, I've been wondering about the default value for the switch_dim parameter. Currently it's 0, which is convenient for 1D-inputs and should be backwards compatible with changepoint models which currently already work. Making it a required parameter would break that backwards compatibility, but would make it more explicit for new(er) users. I'm just worried that users with multidimensional inputs might want to use changepoints without thinking much about the dimensions, and run into problems with the current default behavior. Maybe a middle ground would be to keep the current default, but issue a message/warning when changepoints are used with multidimensional inputs? LMKWYT!

@clwgg
Copy link
Author

clwgg commented Jul 27, 2021

[Just tagging this SO question here to remind me to update my response upon merge: https://stackoverflow.com/questions/68209521]

@clwgg
Copy link
Author

clwgg commented Nov 24, 2021

Hi @st--, I just thought I'd bump this to see if there's still interest in merging this? I'm happy to fix anything that needs fixing!

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.

ChangePoints kernel does not support active_dims
2 participants