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

Modify some classes to functions and struct in DEM #1232

Merged
merged 13 commits into from
Aug 11, 2024
Merged

Conversation

acdaigneault
Copy link
Collaborator

@acdaigneault acdaigneault commented Aug 8, 2024

Description

Some classes should not has been classes but functions or struct.
I modified those classes to a more coherent architechture.
I also gathered all the particle-object contact info structures in one file because it is more convenient that having 3 files for small structs.

Testing

Unit tests have been change according to that.

Miscellaneous (will be removed when merged)

I do not think that needs a change log.
The particle-wall contact info needs to be changed as other, but I will do it when modifying the particle-wall forces.

Checklist (will be removed when merged)

See this page for more information about the pull request process.

Code related list:

  • All in-code documentation related to this PR is up to date (Doxygen format)
  • Lethe documentation is up to date
  • The branch is rebased onto master
  • Code is indented with indent-all and .prm files (examples and tests) with prm-indent
  • If parameters are modified, the tests and the documentation of examples are up to date
  • Changelog (CHANGELOG.md) is up to date if the refactor affects the user experience or the codebase

Pull request related list:

  • No other PR is open related to this refactoring
  • Labels are applied
  • There are at least 2 reviewers (or 1 if small feature) excluding the responsible for the merge
  • If this PR closes an issue or is related to a project, it is linked in the "Projects" or "Development" section
  • If any future works is planed, an issue is opened
  • The PR description is cleaned and ready for merge

Copy link
Contributor

@blaisb blaisb left a comment

Choose a reason for hiding this comment

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

Some minor comments. I will continue reading tomorrow but I doubt I will have many comments

Comment on lines 32 to 33
* of a cell in a list (up to 4 vertices can have the same cell in common in
* 3D). 2 types of container are used for cell neighbors : local-local cells
Copy link
Contributor

Choose a reason for hiding this comment

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

isnt that up to 8 in 3d? Maybe there's a way to make this paragraph a bit clearer :).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok! I did not modify the text here, but I can take a look so it makes more sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree the description is not that obvious, but this is still a good summary. The comments in the function itself are quite explicit tho

std::make_tuple(particles_in_cell_iterator,
first_vertex_location,
second_vertex_location)});
particle_line_contact_candidates.emplace(
Copy link
Contributor

Choose a reason for hiding this comment

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

good usage of emplace here. nice

include/dem/particle_particle_broad_search.h Outdated Show resolved Hide resolved
Copy link
Contributor

@blaisb blaisb left a comment

Choose a reason for hiding this comment

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

last few comments. This is very good work. Thanks for this amazing refactor. It's much easier to read now.


private:
// Broad search objects
ParticleParticleBroadSearch<dim> particle_particle_broad_search_object;
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool to get rid of all these objects :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So many!

include/dem/find_cell_neighbors.h Outdated Show resolved Hide resolved
@blaisb
Copy link
Contributor

blaisb commented Aug 8, 2024

Some tests seem to be crashing but it seems to be due to a deal.II update. I'll try to work on this tomorrow AM

@acdaigneault acdaigneault requested a review from voferreira August 8, 2024 03:27
@blaisb
Copy link
Contributor

blaisb commented Aug 8, 2024

@acdaigneault can you rebase, this way we will be able to see what is breaking because of previous bugs and not

@blaisb
Copy link
Contributor

blaisb commented Aug 8, 2024

@acdaigneault I'm not sure what is this hang error you are getting, but it seems something is off since it happens in both release and debug?

@acdaigneault
Copy link
Collaborator Author

@acdaigneault I'm not sure what is this hang error you are getting, but it seems something is off since it happens in both release and debug?

Yes this is related to my PR

Copy link
Collaborator

@voferreira voferreira left a comment

Choose a reason for hiding this comment

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

All clear to me. Cleaning is so satisfying haha. Great job!

@blaisb blaisb merged commit ce0736b into master Aug 11, 2024
8 checks passed
@blaisb blaisb deleted the dem_remove-classes branch August 11, 2024 00:06
M-Badri pushed a commit to M-Badri/lethe that referenced this pull request Sep 29, 2024
Description
Some classes should not has been classes but functions or struct.
I modified those classes to a more coherent architechture.
I also gathered all the particle-object contact info structures in one file because it is more convenient that having 3 files for small structs.

Testing
Unit tests have been change according to that.

Former-commit-id: ce0736b
blaisb pushed a commit that referenced this pull request Sep 30, 2024
Description
Some classes should not has been classes but functions or struct.
I modified those classes to a more coherent architechture.
I also gathered all the particle-object contact info structures in one file because it is more convenient that having 3 files for small structs.

Testing
Unit tests have been change according to that.

Former-commit-id: ce0736b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Refactoring This PR is only refactoring or clean up Reviewed and ready to merge
Projects
None yet
3 participants