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

Parameters with constraints #106

Closed

Conversation

HamletWantToCode
Copy link
Contributor

Hi @willtebbutt, the main feature of this PR is the Parameter type as we discussed #104 . Parameter receives scalar/vector/matrix and use bijector in Bijectors.jl to restrict their domain. I modify several constructor of kernels so that people can use it without changing their existing code. Here are some example:

# build a parameter wrapper that do nothing
Parameter(3.0)
# build a parameter that only receive positive inputs
Parameter(3.0, Val(:pos))
Parameter(-3.0, Val(:pos))  # will throw a DomainError
# build a parameter that receive inputs with (0, 2)
using Bijectors: Logit
Parameter(0.5, inv(Logit(0, 2)))

# proper value of the parameter can be retrieved by `value`
value(Parameter(3.0)) == 3.0

# also allows pass in vector/matrix as parameter
Parameter(randn(4))
Parameter(rand(3), Val(:pos))

# a strided and scaled kernel
k = 3.0 * stretch(EQ(), 1.0)

# if you use Flux.jl, you can use `params` to collect parameters for unconstrained optimizer
params(k)

Currently, I only modify code in src/gp/ folder and don't touch src/composite folder, I will proceed if we both happy with the APIs.

In #104 , you proposed to use

struct Positive{Tx, Tf} <: AbstractParameter
   x::Ref{Tx}
   f::Tf
   Positive(y, f) = new{Tx, Tf}(Ref(inv(f)(y)), f)
end

and indicated that "the use of a Ref for Flux compatibility". However, if Ref is used, then params can't extract x. params of Flux requires trainable parameters to be an AbstractArray, so, currently x is heap allocated ( maybe I miss something, if so, please let me know ).

HamletWantToCode and others added 23 commits March 13, 2020 23:57
@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #106 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #106   +/-   ##
=======================================
  Coverage   82.89%   82.89%           
=======================================
  Files          27       27           
  Lines         795      795           
=======================================
  Hits          659      659           
  Misses        136      136           

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 12ebb37...12ebb37. Read the comment docs.

@willtebbutt
Copy link
Member

Thanks for the work @HamletWantToCode -- I'm really busy at the minute, but will try and get around to reviewing in the next few days.

@willtebbutt
Copy link
Member

@HamletWantToCode I'm really sorry for letting this drop off my radar.

I was wondering if you'd be open to spinning this out into a separate package? I would be very happy to make Stheno depend on it of course -- I just think that the community could benefit from something like this and it would be a bit of a shame to lock it up in Stheno.

@HamletWantToCode
Copy link
Contributor Author

Never mind. I'd love to make it a separate package. Maybe I will do it next week, after I finish my assignments.

@HamletWantToCode
Copy link
Contributor Author

Should we close this PR ?

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