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

test only PolyChaos #402

Closed

Conversation

vikram-s-narayan
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #402 (35d9e8b) into master (f78179a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #402   +/-   ##
=======================================
  Coverage   46.56%   46.56%           
=======================================
  Files          16       16           
  Lines        2564     2564           
=======================================
  Hits         1194     1194           
  Misses       1370     1370           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@vikram-s-narayan
Copy link
Contributor Author

@ChrisRackauckas - tests pass if we change Zygote = "0.4, 0.5, 0.6" to Zygote = "= 0.6.40" in project.toml
Would it be okay to force the Zygote version to 0.6.40 for now?

@ChrisRackauckas
Copy link
Member

Only force a version if an upstream issue with an MWE has been made, and then open an issue here to track it. Has that upstream issue been made?

@vikram-s-narayan
Copy link
Contributor Author

Only force a version if an upstream issue with an MWE has been made, and then open an issue here to track it.

Okay.

Has that upstream issue been made?

I'm still researching how to create an MWE. The SurrogatesPolyChaos tests break when we move from Zygote Version 0.6.43 to 0.6.44 which happened around August 1st. Perhaps this discussion is related?

Also, it appears that from version 0.6.42 itself, the following warning is being given:
"Zygote.@Nograd myfuncis deprecated, useChainRulesCore.@non_differentiable myfunc(::Any...)` instead."

In addition, I'm trying to understand why a GEK surrogate has been created in a test for PolyChaos.

The line that is causing the test failure is the third one below:

        my_gek_ND = GEK(x, y, lb, ub)
        g = x -> Zygote.gradient(my_gek_ND, x)
        g((2.0, 5.0)) #this is the line that triggers the error

@ChrisRackauckas
Copy link
Member

Just get a version of the test that's failing into a copy-pastable form with the version information and open a Zygote.jl issue with that. The problem is that if we block the patch release without ever upstreaming the issue, that's not a real fix as it will never get fixed. If there is an upstream issue, at least it can start to be discussed and worked on.

@vikram-s-narayan
Copy link
Contributor Author

Just get a version of the test that's failing into a copy-pastable form with the version information and open a Zygote.jl issue with that. The problem is that if we block the patch release without ever upstreaming the issue, that's not a real fix as it will never get fixed. If there is an upstream issue, at least it can start to be discussed and worked on.

I have opened a Zygote.jl issue.

@ChrisRackauckas
Copy link
Member

This isn't the one to merge: is there a separate PR setup to merge with green tests?

@vikram-s-narayan
Copy link
Contributor Author

This isn't the one to merge: is there a separate PR setup to merge with green tests?

#394 is the right one to merge. I'll be making another commit after formatting because the formatting test is failing.

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