SciML / Surrogates.jl

Surrogate modeling and optimization for scientific machine learning (SciML)
https://docs.sciml.ai/Surrogates/stable/
Other
328 stars 70 forks source link

test only PolyChaos #402

Closed vikram-s-narayan closed 1 year ago

codecov[bot] commented 2 years ago

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           

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

vikram-s-narayan commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

vikram-s-narayan commented 1 year ago

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

https://github.com/SciML/Surrogates.jl/pull/394 is the right one to merge. I'll be making another commit after formatting because the formatting test is failing.