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

St/dw/scalar parameterhandling/merge #419

Open
wants to merge 7 commits into
base: dw/scalar_parameterhandling
Choose a base branch
from

Conversation

st--
Copy link
Member

@st-- st-- commented Dec 23, 2021

Merges master back into #397

st-- and others added 7 commits November 24, 2021 17:55
* restrict Zygote to <0.6.30

* revert Zygote test restriction and add finer-grained testset

* Update test/utils.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* revert testset

* mark test_broken

* Use `@test_throws` instead of `@test_broken`

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: David Widmann <[email protected]>
* Fix typo in valid_inputs error

* Update src/utils.jl

Co-authored-by: David Widmann <[email protected]>

Co-authored-by: David Widmann <[email protected]>
Zygote AD failures:

* revert #409 (test_utils workaround for broken Zygote - now working again)

* disable broken Zygote AD test for ChainTransform

Improved tests:

* finer-grained testsets

* add missing test cases to test_AD

* replace test_FiniteDiff with test_AD(..., :FiniteDiff, ...)

* remove code duplication
* use only() instead of first() for 1-"vectors" that were for the benefit of Flux

* fix one test that should not have worked as it was

* add missing scalar Sinus constructor
…e test, (keep existing compat) (#418)

Co-authored-by: CompatHelper Julia <[email protected]>
…ompat) (#411)

Co-authored-by: CompatHelper Julia <[email protected]>
Co-authored-by: Théo Galy-Fajou <[email protected]>
Co-authored-by: st-- <[email protected]>
…tions.jl into st/dw/scalar_parameterhandling/merge
@st-- st-- requested a review from devmotion December 23, 2021 12:20
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

The branch is outdated, I have additional changes and fixes locally. So unfortunately it's not helpful ro merge this PR currently. I'm sorry but exactly such problems I wanted to avoid, I still think it was a bad idea to make these changes.

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #419 (b36342d) into dw/scalar_parameterhandling (2ccf8cf) will increase coverage by 0.33%.
The diff coverage is 20.00%.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           dw/scalar_parameterhandling     #419      +/-   ##
===============================================================
+ Coverage                        12.62%   12.96%   +0.33%     
===============================================================
  Files                               53       53              
  Lines                             1386     1396      +10     
===============================================================
+ Hits                               175      181       +6     
- Misses                            1211     1215       +4     
Impacted Files Coverage Δ
src/basekernels/fbm.jl 0.00% <0.00%> (ø)
src/basekernels/matern.jl 0.00% <0.00%> (ø)
src/distances/sinus.jl 54.54% <0.00%> (-5.46%) ⬇️
src/utils.jl 33.78% <ø> (+0.90%) ⬆️
src/kernels/transformedkernel.jl 35.55% <50.00%> (ø)
src/transform/ardtransform.jl 75.00% <100.00%> (ø)
src/zygoterules.jl 33.33% <0.00%> (-26.67%) ⬇️
src/transform/chaintransform.jl 47.36% <0.00%> (+1.42%) ⬆️
src/chainrules.jl 7.59% <0.00%> (+2.40%) ⬆️
... and 1 more

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 2ccf8cf...b36342d. Read the comment docs.

@st--
Copy link
Member Author

st-- commented Dec 24, 2021

The branch is outdated, I have additional changes and fixes locally. So unfortunately it's not helpful ro merge this PR currently. I'm sorry but exactly such problems I wanted to avoid, I still think it was a bad idea to make these changes.

Well, it wasn't any work; let me know once you've pushed your local changes, I'm happy to re-do it. I still think it's not a big problem and that it was better to get the other PR in as it is.

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.

4 participants