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

Improved GEKPLS Function #456

Closed
wants to merge 6 commits into from
Closed

Improved GEKPLS Function #456

wants to merge 6 commits into from

Conversation

Spinachboul
Copy link
Contributor

@Spinachboul Spinachboul commented Dec 21, 2023

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.

Copy link

codecov bot commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aa744e5) 78.12% compared to head (089c699) 78.12%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #456   +/-   ##
=======================================
  Coverage   78.12%   78.12%           
=======================================
  Files          23       23           
  Lines        3154     3154           
=======================================
  Hits         2464     2464           
  Misses        690      690           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Spinachboul
Copy link
Contributor Author

@ChrisRackauckas
I have changed the sampling method from Sobol to Halton but the PR is not passing the checks
Could you suggest any changes??
Here is the link to the GitHub repo: https://github.com/Spinachboul/Surrogates.jl/tree/master


# Modifications for Improved GEKPLS Function:

To enhance the GEKPLS function, sampling method was changed from ```SobolSample()``` to ```HaltonSample()```.
Copy link
Member

Choose a reason for hiding this comment

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

That's not an enhancement, that's just a change in the sampling method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but gekpls.jl might improve if we use Halton sampling instead of sobol sampling.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but that's a user choice. That's not inherent to gekpls but just an option for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Multiple choices might work for multiple applications. But at the core, we should focus on optimizing complex mathematical functions like matrix multiplication and also focus on optimal featurization which works better for real world applications.

Yes, but that's a user choice. That's not inherent to gekpls but just an option for it.

Copy link
Member

Choose a reason for hiding this comment

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

What's your proof this is a better default? Can you show some head-to-head benchmarks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recently hit a PR, where I proposed changes in the readme file of tensor_prod.md
#457
But I think the kind of example functions that we are using are also not completely justified for the benchmark label.
We should use better examples to explain the surrogate optimization and as @sathvikbhagavan rightly said, we need to dive deeper into explaining the mathematical concepts.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, changes in tensor_prod.md is in the right direction.

Copy link
Contributor Author

@Spinachboul Spinachboul Dec 29, 2023

Choose a reason for hiding this comment

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

@sathvikbhagavan
So is it ready to merge or still do you think it can be changed at a few spots? Like I think I did not explain mathematically about the function and just textually explained the function.
Should I add in the whole concept image available online?
Link: https://projecteuclid.org/journals/tohoku-mathematical-journal/volume-17/issue-2/The-tensor-product-of-function-algebras/10.2748/tmj/1178243579.pdf

Copy link
Member

Choose a reason for hiding this comment

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

Can you clean up the PRs - use either this or #457 and close one of them, remove the extra tutorial which is not needed and then we can have a round of reviews?

Copy link
Contributor Author

@Spinachboul Spinachboul Dec 29, 2023

Choose a reason for hiding this comment

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

@sathvikbhagavan and @ChrisRackauckas
Yeah so let's close this one as I am still experimenting with the sampling methods for better rmse!!
Will maybe again hit a PR for the same.

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