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

GEKPLS With Custom PLS Based on SKLearn PLS #359

Merged
merged 12 commits into from
Jun 29, 2022

Conversation

vikram-s-narayan
Copy link
Contributor

@vikram-s-narayan vikram-s-narayan commented Jun 15, 2022

This version of GEKPLS replaces the SKLearn PLS in the previous version with a modified PLS version based on the SKLearn PLS code. I have also added more tests, additional functionality and documentation.

It does not include code equivalent to SMT's hyperparameter optimization for theta. I plan to work on hyperparameter optimization next as I'm still researching how best to implement it. I intend to submit a separate pull request for hyperparameter optimization.

@vikram-s-narayan vikram-s-narayan changed the title GEKPLS with own pls GEKPLS With Custom PLS Based on SKLearn PLS Jun 15, 2022
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #359 (cd72b3f) into master (f7d2447) will increase coverage by 1.30%.
The diff coverage is 92.16%.

@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
+ Coverage   77.59%   78.89%   +1.30%     
==========================================
  Files          15       16       +1     
  Lines        2053     2270     +217     
==========================================
+ Hits         1593     1791     +198     
- Misses        460      479      +19     
Impacted Files Coverage Δ
src/Surrogates.jl 18.18% <ø> (ø)
src/GEKPLS.jl 92.16% <92.16%> (ø)
src/Optimization.jl 72.16% <0.00%> (-0.19%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@vikram-s-narayan vikram-s-narayan marked this pull request as ready for review June 21, 2022 11:09
@vikram-s-narayan
Copy link
Contributor Author

Also, I ran a quick comparison between GEKPLS, Kriging and Radial Basis.

Results for the water flow function are below:

rmse_rad: 50.399
rmse_krig: 46.086
rmse_gekpls: 0.0185

@ChrisRackauckas
Copy link
Member

Wow that's awesome!

@ChrisRackauckas ChrisRackauckas requested a review from ranjanan June 22, 2022 10:56
@ranjanan
Copy link
Contributor

ranjanan commented Jun 22, 2022

nice! Also compare PolyChaos? like in the docs: http://surrogates.sciml.ai/stable/water_flow/? Curious how that stacks up as well.

@vikram-s-narayan
Copy link
Contributor Author

nice! Also compare PolyChaos? like in the docs: http://surrogates.sciml.ai/stable/water_flow/? Curious how that stacks up as well.

rmse_poly: 1.518

Updated gist here.

@ChrisRackauckas
Copy link
Member

Set some strict modes in the doc build: https://github.com/SciML/DiffEqSensitivity.jl/blob/v6.79.0/docs/make.jl#L16-L23

@ChrisRackauckas
Copy link
Member

@ranjanan are you doing the review?

src/GEKPLS.jl Outdated Show resolved Hide resolved
src/GEKPLS.jl Show resolved Hide resolved
src/GEKPLS.jl Outdated Show resolved Hide resolved
src/GEKPLS.jl Outdated Show resolved Hide resolved
src/GEKPLS.jl Outdated Show resolved Hide resolved
@ranjanan
Copy link
Contributor

Ah review didn't get posted. Posted now.

@vikram-s-narayan
Copy link
Contributor Author

Set some strict modes in the doc build: https://github.com/SciML/DiffEqSensitivity.jl/blob/v6.79.0/docs/make.jl#L16-L23

I'm currently getting an error ("ERROR: makedocs encountered an error (:docs_block, :example_block). Terminating build before rendering.") without being able to create the build directory when I make the following change in make.jl:

makedocs(
    sitename="Surrogates.jl",

    strict = [
             :doctest,
             :linkcheck,
             :parse_error,
             :example_block,
             # Other available options are
             # :autodocs_block, :cross_references, :docs_block, :eval_block, :example_block, :footnote, :meta_block, :missing_docs, :setup_block
         ],
         
    format = Documenter.HTML(analytics = "UA-90474609-3",
                         assets = ["assets/favicon.ico"],
                         canonical="https://surrogates.sciml.ai/stable/"),
    pages = pages
)

So my understanding is that each page in docs will need to be fixed independently in order to make these tests pass. I will work on these separately and send in other pull requests for documentation not related to GEKPLS.

@ChrisRackauckas
Copy link
Member

So my understanding is that each page in docs will need to be fixed independently in order to make these tests pass. I will work on these separately and send in other pull requests for documentation not related to GEKPLS.

Yes that's fine.

Note you may need to run the formatter to fix the conflicts. Just do:

using JuliaFormatter, Surrogates
format(joinpath(dirname(pathof(Surrogates)), ".."))

@ChrisRackauckas
Copy link
Member

I think this is generally good to merge after conflicts are handled. We can do the docs stuff in a separate PR. And we should open an issue to profile and optimize the implementation, but I don't think there's any major blocking in the code here.

@vikram-s-narayan
Copy link
Contributor Author

vikram-s-narayan commented Jun 28, 2022

I ran the JuliaFormatter but the format-check still fails. Assuming that I've run the formatting code correctly, my guess is that this format-check fail is happening because newly added and modified Julia files will always show up in the git diff

So the condition in the following code may need to be altered?


  julia -e '
  out = Cmd(`git diff --name-only`) |> read |> String
  if out == ""
      exit(0)
  else
      @error "Some files have not been formatted !!!"
      write(stdout, out)
      exit(1)
  end'

@ChrisRackauckas
Copy link
Member

No. You ran the formatter before you rebased to master, so you ran it with the wrong style because it didn't have the JuliaFormatter.toml file in there. Revert that one (because it did a bunch of spacing changes that aren't preferred but aren't against the style, so they would be kept) and run the formatter on a post-rebased commit.

Project.toml Outdated
@@ -8,9 +8,11 @@ Distributions = "31c24e10-a181-5473-b8eb-7969acd0382f"
ExtendableSparse = "95c220a8-a1cf-11e9-0c77-dbfce5f500b3"
GLM = "38e38edf-8417-5370-95a0-9cbb8c7f171a"
IterativeSolvers = "42fd0dbc-a981-5370-80f2-aaf504508153"
JuliaFormatter = "98e50ef6-434e-11e9-1051-2b60c6c9e899"
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be required in the Project?

Project.toml Outdated Show resolved Hide resolved
@ChrisRackauckas ChrisRackauckas merged commit 80a1a97 into SciML:master Jun 29, 2022
@ChrisRackauckas
Copy link
Member

🎉 thanks for seeing this through, this is a huge step.

@ChrisRackauckas
Copy link
Member

oh I noticed the the formatter CI is gone. I'll just add it back and handle any issue there.

@vikram-s-narayan vikram-s-narayan deleted the gekpls_with_own_pls branch June 29, 2022 10:11
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.

3 participants