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

Check if particles are overlapping #41

Merged
merged 4 commits into from
Aug 7, 2023

Conversation

Kevish-Napal
Copy link
Contributor

Hey, finally found how to interact with Pkg and git correctly!

I wrote the first function isempty(Origins,Radii) where Origins and Radii are vectors so one can easily check this function without having to define particles.

Kevish

Copy link
Member

@jondea jondea left a comment

Choose a reason for hiding this comment

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

Hi @Kevish-Napal, massive thanks for your contribution, this looks like a useful check, so thank you for implementing it. Also, great work fighting Pkg and git, I promise you're almost there to your first PR!

One overall comment is that the convention in this package (and most Julia packages) is to use snake_case for variable names and CamelCase for types and modules. Otherwise, this looks like a great first PR.

src/simulation.jl Outdated Show resolved Hide resolved
src/simulation.jl Outdated Show resolved Hide resolved
src/simulation.jl Outdated Show resolved Hide resolved
Copy link
Member

@jondea jondea left a comment

Choose a reason for hiding this comment

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

Thank you for your changes! Two of the comments weren't full addressed, I've tried to add a bit more detail, let me know if your have any questions.

src/simulation.jl Outdated Show resolved Hide resolved
src/simulation.jl Outdated Show resolved Hide resolved
Copy link
Member

@jondea jondea left a comment

Choose a reason for hiding this comment

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

It looks good to me, thank you again! I'll let @arturgower have a quick look over and merge it if he's happy.

@arturgower arturgower merged commit 752c5fc into JuliaWaveScattering:master Aug 7, 2023
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