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

Adding other KAN layers #3

Merged
merged 17 commits into from
Oct 31, 2024
Merged

Adding other KAN layers #3

merged 17 commits into from
Oct 31, 2024

Conversation

armbrusl
Copy link
Contributor

I have implemented the following two layers

Julia implementation of FourierKAN
Julia implementation of ChebyKAN

I also added another test for all the layers. Not sure how most of GitHub works so let me know if there is anything else I should do.

.gitignore Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@vpuri3
Copy link
Owner

vpuri3 commented Oct 29, 2024

Thanks @armbrusl, the code looks ok. Can you

  • take a look at the comments
  • update the README code to include the new layers

@vpuri3
Copy link
Owner

vpuri3 commented Oct 29, 2024

just pushed a commit to master with all the Lux 0.x --> 1.x fixes. @armbrusl I also rebased this branch to master. I removed all the unnecessary packages in test/Project.toml (e.g., LLVM, Debugger, etc). They should not be added to the pull request. You should do a git fetch && git pull on your machine before proceeding. You should also update your Julia environment.

Can you also move the test you added in test/runtests.jl to a separate file.

src/fdense.jl Show resolved Hide resolved
src/cdense.jl Show resolved Hide resolved
@armbrusl
Copy link
Contributor Author

I think I have addressed your suggestions. Thanks for walking me through it! Let me know if there is anything else I need to do. The only thing I haven't done is edit the README file, is there anything specific I should add ?

@vpuri3
Copy link
Owner

vpuri3 commented Oct 29, 2024

see above comments

# Batched Multiplication using Einstein Summation
#======================================================#

@inline function batched_mul(x, coeffs)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you not use NNlib.batched_mul or NNlib.batched_vec for this?

Copy link
Owner

Choose a reason for hiding this comment

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

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 am not sure. I have been trying but I have not able to get it right

@vpuri3
Copy link
Owner

vpuri3 commented Oct 29, 2024

Can you add the new layers to the code in the README and report timings?

https://github.com/vpuri3/KolmogorovArnold.jl/blob/master/README.md?plain=1#L14-L88

If not, I can do it after this is merged.

@vpuri3
Copy link
Owner

vpuri3 commented Oct 29, 2024

See workflow error: https://github.com/vpuri3/KolmogorovArnold.jl/actions/runs/11579597184/job/32236568354?pr=3#step:6:545

@vpuri3
Copy link
Owner

vpuri3 commented Oct 30, 2024

@armbrusl can you investigate this error? I am not seeing it in master with 1.10 or 1.11. I also updated master to only test on 1.10 and 1.11. If you rebase this branch, then that change will be incorporated.

@armbrusl
Copy link
Contributor Author

@vpuri3 I am not able to replicate the errors, when I run the required command

import Pkg; Pkg.test(;coverage=true, julia_args=["--check-bounds=yes", "--compiled-modules=yes"], force_latest_compatible_version=false, allow_reresolve=true)

everything works fine. I also rebased the branch.

@vpuri3
Copy link
Owner

vpuri3 commented Oct 31, 2024

@armbrusl i tested the CI on Master in #4 and it is fine. The failures here are due to changes introduced in this pr

I also don't see your rebase commits. Make sure you git checkout master, git fetch and git pull the latest version. then checkout this branch again and git merge master. and then do a git push

@armbrusl
Copy link
Contributor Author

I think I have done the steps you told me, how will I know if I have done it right

@vpuri3
Copy link
Owner

vpuri3 commented Oct 31, 2024

I'll check out this branch and investigate this evening

@vpuri3
Copy link
Owner

vpuri3 commented Oct 31, 2024

@armbrusl you haven't rebased correctly. Look at this branch. This branch is some commits behind. You need to rebase this branch to the latest master.

image

@vpuri3 vpuri3 merged commit e9b4134 into vpuri3:master Oct 31, 2024
2 checks passed
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