search

TuringLang / Bijectors.jl

Implementation of normalising flows and constrained random variable transformations
https://turinglang.org/Bijectors.jl/
MIT License
196 stars 32 forks source link

Numerical improvements to correlation bijectors #313

Closed sethaxen closed 1 month ago

sethaxen commented 1 month ago

This PR implements the numerical suggestions in #301. It does not make any of the suggested renaming changes, which will be left for a future PR.

sethaxen commented 1 month ago

Remaining errors seem to be ones introduced in #304 and unrelated to this PR

torfjelde commented 1 month ago

Remaining errors seem to be ones introduced in https://github.com/TuringLang/Bijectors.jl/pull/304 and unrelated to this PR

Uhmm that's strange o.O Don't understand why this wasn't failing in the original PR. Ooor it might be because it hit the cholesky error and thus didn't run the interface tests on 1.6.. Should be a quick Compat.jl inclusion though; lemme have a check

yebai commented 1 month ago

It seems #314 does not address the issue on Julia 1.6.

sethaxen commented 1 month ago

I don't think #314 was completed before merging. Its CI was still failing with a similar error.

torfjelde commented 1 month ago

Yes, #314 was indeed not ready for a merge @yebai ; why was it merged?

yebai commented 1 month ago

Yes, https://github.com/TuringLang/Bijectors.jl/pull/314 was indeed not ready for a merge @yebai ; why was it merged?

my bad -- it shouldn't have been merged.

torfjelde commented 1 month ago

@sethaxen I just pushed the fix directly to this branch. Let's see if CI succeeds now:)

torfjelde commented 1 month ago

Damn, even this doesn't work because eachslice only supports a single value as the dims arg :confused:

Really sorry about this @sethaxen ; this bug was hidden behind an unrelated numerical issue that caused this particular test to never be run on 1.6. But the cause of this shouldn't have been merged.

I'll just disable those tests on this PR and then we'll have to fix it in a separate PR.

sethaxen commented 1 month ago

Seems like it worked!

EDIT: and, no problem, @torfjelde !

torfjelde commented 1 month ago

Lovely! Feel free to hit the big green button:)

sethaxen commented 1 month ago

Sadly, I am not an "authorized user," and the button is gray.

torfjelde commented 1 month ago

Want me to do it then? Also happy to let give you authorization given your involvement in Bijectors.jl if you want to:)

sethaxen commented 1 month ago

Want me to do it then? Also happy to let give you authorization given your involvement in Bijectors.jl if you want to:)

Sure to both!

torfjelde commented 1 month ago

Done:) Wonderful stuff @sethaxen ; thanks!