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

Convolutions in 1d in rust #1007

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

eengamer2007
Copy link

implemented the convolutions 1d in rust

@Amaras Amaras added Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) lang: rust Rust programming language labels Nov 20, 2022
Copy link
Member

@Amaras Amaras left a comment

Choose a reason for hiding this comment

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

Hi, thank you for your interest in the AAA.
I've run clippy (with -W clippy::pedantic as well) on your code and put all its suggestions here, as well as some suggestions and questions of mine.

I also don't like the fact that you cast a lot between usize and isize in convolve_linear and convolve_cyclic, but I'm not sure how you could do otherwise without increasing complexity (and it makes pedantic clippy complain a lot)

contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
contents/convolutions/1d/code/rust/1d_convolution.rs Outdated Show resolved Hide resolved
eengamer2007 and others added 7 commits November 21, 2022 19:16
Not sure why I didn't do it in the first place.

Co-authored-by: Sammy Plat <[email protected]>
Co-authored-by: Sammy Plat <[email protected]>
@eengamer2007
Copy link
Author

eengamer2007 commented Nov 22, 2022

i saw somewhere that I should add my name to the contributers list since this is my first contribution. Should that be in this pull request?

@leios
Copy link
Member

leios commented Nov 22, 2022

Yeah, add that to this pr, if you can

@Amaras
Copy link
Member

Amaras commented Jan 13, 2023

Oof, sorry for the delay in the response.

So, the main problem with your PR currently is that the line numbers don't track correctly.
I don't see a reason to refuse your code in the AAA now, so please make sure the lines you include your code at are indeed the correct ones before I merge them.

@eengamer2007
Copy link
Author

sorry for the late fix, school was keeping me busy.

@eengamer2007 eengamer2007 requested a review from Amaras February 10, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) lang: rust Rust programming language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants