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

fix type error in grid generators and add some test functions for gri… #973

Merged

Conversation

Abdelrahman912
Copy link
Contributor

Summary

This PR fixes type conversion issues in the generate_grid function in grid_generators.jl and adds test coverage for all grid generators.

Tests

  • Added new test cases in test/test_grid_generators.jl.
  • Verified that all tests pass locally.

@Abdelrahman912 Abdelrahman912 marked this pull request as ready for review June 4, 2024 20:25
@termi-official
Copy link
Member

Thanks for putting this together @Abdelrahman912 ! I left a few minor comments. It should be good to go afterwards.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.69%. Comparing base (c33a191) to head (a93ca25).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #973   +/-   ##
=======================================
  Coverage   93.69%   93.69%           
=======================================
  Files          36       36           
  Lines        5438     5440    +2     
=======================================
+ Hits         5095     5097    +2     
  Misses        343      343           

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

Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

Nice 🎉
Just two small drive-by comments from me

  1. I think it is more logical to take the type parameter from the node than the left point (even if this is equivalent, but seeing that requires looking at the call site)
  2. I think it is better to convert and then do division by integer. For Float32 it doesn't matter (only perf), but for higher precision floats it makes a difference

src/Grid/grid_generators.jl Outdated Show resolved Hide resolved
src/Grid/grid_generators.jl Outdated Show resolved Hide resolved
src/Grid/grid_generators.jl Outdated Show resolved Hide resolved
src/Grid/grid_generators.jl Outdated Show resolved Hide resolved
Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

Nice, just left some comments on the testing

test/test_grid_generators.jl Outdated Show resolved Hide resolved
test/test_grid_generators.jl Outdated Show resolved Hide resolved
test/test_grid_generators.jl Outdated Show resolved Hide resolved
src/Grid/grid_generators.jl Outdated Show resolved Hide resolved
Copy link
Member

@KnutAM KnutAM left a comment

Choose a reason for hiding this comment

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

Great work on your first PR @Abdelrahman912 🚀

Content-wise this looks good to me now, just some formatting fixes as commented below, and the pre-commit action should pass. See #928 for detail on how to do that automatically.

src/Grid/grid_generators.jl Outdated Show resolved Hide resolved
src/Grid/grid_generators.jl Outdated Show resolved Hide resolved
test/test_grid_generators.jl Outdated Show resolved Hide resolved
@fredrikekre fredrikekre merged commit 44e43d8 into Ferrite-FEM:master Jun 7, 2024
10 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.

5 participants