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

Dem volume insertion initial velocities #1078

Merged
merged 4 commits into from
Mar 29, 2024

Conversation

OGaboriault
Copy link
Collaborator

Description of the problem

The initial velocity and initial angular velocity are now define with one parameter each.

Really quick PR.

No example or application test (except for one) was using those parameters.

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.

Are you sure there are no examples that use this ? I am thinking about the particle bouncing on a wall one. Can you double check that no examples are affected? Otherwise, all good for me.

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.

Good job! Only one comment about the insertion_info, but nothing special.

double vel_x = dem_parameters.insertion_info.initial_vel[0];
double vel_y = dem_parameters.insertion_info.initial_vel[1];
double vel_z = dem_parameters.insertion_info.initial_vel[2];
double omega_x = dem_parameters.insertion_info.initial_ang_vel[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the new parameter name, but I think I would rather keep dem_parameters.insertion_info.omega[0], because the rest of the code uses omega. Otherwise it is all clear to me :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, and you don't need initial, since this is insertion info

Copy link
Collaborator Author

@OGaboriault OGaboriault Mar 29, 2024

Choose a reason for hiding this comment

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

I think the initial is useful. This way, there's not confusion with the velocity at a given time step.

I can do initial_omega. I agree, omega is more coherent with the rest of code.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, I tend to prefer long meaningful names instead of confusing abbreviations, but in this case I found it slightly redundant, hence, not necessary. This is not a strong opinion, though. So this is really up to you :)

@blaisb blaisb merged commit 698b063 into master Mar 29, 2024
8 checks passed
@blaisb
Copy link
Contributor

blaisb commented Mar 29, 2024

Ohh shoot, did not see the comments before the merge. You can fix them in a secondary quick PR @OGaboriault

@blaisb blaisb deleted the dem_volume_insertion_initial_velocities branch March 29, 2024 13:06
blaisb pushed a commit that referenced this pull request Mar 29, 2024
M-Badri pushed a commit to M-Badri/lethe that referenced this pull request Sep 29, 2024
Description of the problem
This is a follow up from Victor's comment from PR [Dem volume insertion box parameters chaos-polymtl#1074
The initial velocity and initial angular velocity are now define with one parameter each.

Really quick PR.

No example or application test (except for one) was using those parameters.



Former-commit-id: 698b063
M-Badri pushed a commit to M-Badri/lethe that referenced this pull request Sep 29, 2024
blaisb pushed a commit that referenced this pull request Sep 30, 2024
Description of the problem
This is a follow up from Victor's comment from PR [Dem volume insertion box parameters #1074
The initial velocity and initial angular velocity are now define with one parameter each.

Really quick PR.

No example or application test (except for one) was using those parameters.



Former-commit-id: 698b063
blaisb pushed a commit that referenced this pull request Sep 30, 2024
Follow-up to PR #1078 ...

Former-commit-id: 13e9dcb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants