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

Working on Bingham Material #13

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

Conversation

SachinJalan
Copy link

Issue #9

Copy link
Collaborator

@chahak13 chahak13 left a comment

Choose a reason for hiding this comment

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

Use black to format the code with line length 88

diffmpm/material.py Outdated Show resolved Hide resolved
@SachinJalan
Copy link
Author

I had a doubt in bingham.tcc file, in the function compute stress
Eigen::Matrix<double, 6, 1> mpm::Bingham<Tdim>::compute_stress( const Vector6d& stress, const Vector6d& dstrain, const ParticleBase<Tdim>* ptr, mpm::dense_map* state_vars)

can you please tell what is the state_vars.

@SachinJalan SachinJalan requested a review from chahak13 July 2, 2023 20:15
@SachinJalan SachinJalan marked this pull request as draft July 2, 2023 20:49
@chahak13
Copy link
Collaborator

chahak13 commented Jul 2, 2023

I haven't implemented state variables yet but I think it's just a dictionary. See

https://github.com/cb-geo/mpm/blob/53fcb7609375fd44bf3481d24ee18bfdf0393adb/include/materials/bingham.tcc#L32-L37

@kks32
Copy link
Contributor

kks32 commented Jul 2, 2023

Yes, it's a dict to store variables specific to individual material points

@SachinJalan
Copy link
Author

In the particles.py class the init function does not have dvolumetric_strain while it has been computed in later functions of the same class.

@SachinJalan SachinJalan marked this pull request as ready for review July 3, 2023 15:14
@chahak13
Copy link
Collaborator

chahak13 commented Jul 4, 2023

Thanks for pointing that out. I added it to the init method. The reason it worked fine was that every time a new value is assigned to it so it was not needed to be carried over by jax (un/)flattening but better to have it there.

Copy link
Collaborator

@chahak13 chahak13 left a comment

Choose a reason for hiding this comment

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

Can you also add a couple more things?

  • Add a test case
  • Add docstrings & comments wherever needed to document its working
  • Check dimensions of the variables. (I think I saw some operations that might raise an error)

I've also added a few more comments. Let me know if you have questions or need any help!

diffmpm/material.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
@kks32 kks32 marked this pull request as draft July 7, 2023 12:16
@SachinJalan
Copy link
Author

In the test cases of bingham material I am getting a deviation from the answer present in the CB-Geo test case from bingham in case 3 and 4 which are for yielded case, The tau value is coming off by a value of 20-30 I have checked the code multiple times but am unable to find any error, the dvolumetric strain is coming correct. I think the strain rate has some error it is coming [-0.25,-0.375,0.0,-0.625,0,0] can you please see once

@chahak13
Copy link
Collaborator

@SachinJalan Let me see if I can find something. Can you be a little clearer about where you think the error is stemming from? Trying to pin in the issue - you say dvolumetric_strain is correct but the strain rate isn't? What would be the expected strain rate?

tests/test_bingham.py Outdated Show resolved Hide resolved
diffmpm/material.py Outdated Show resolved Hide resolved
Comment on lines 203 to 204
Arguments
---------
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Arguments
---------
Parameters
----------

Copy link
Collaborator

@chahak13 chahak13 left a comment

Choose a reason for hiding this comment

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

I think we're close to wrapping this up. Just a few small comments.

{"pressure": jnp.zeros((2, 1))}),
],
)
def test_compute_stress_two_particles(particles, state_vars, element, target, dt):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. Just declare dt in the test directly.

Comment on lines 128 to 137
@pytest.fixture
def state_vars():
return {"pressure": jnp.zeros(1)}


@pytest.fixture
def dt():
return 1.0


@pytest.mark.parametrize(
"particles, element, target",
particles_element_targets,
)
def test_compute_stress(particles, state_vars, element, target, dt):
particles.update_natural_coords(element)
if element.constraints:
element.apply_boundary_constraints()
particles.compute_strain(element, dt)
stress = particles.material.compute_stress(None, particles, state_vars)
assert jnp.allclose(stress, target)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@pytest.fixture
def state_vars():
return {"pressure": jnp.zeros(1)}
@pytest.fixture
def dt():
return 1.0
@pytest.mark.parametrize(
"particles, element, target",
particles_element_targets,
)
def test_compute_stress(particles, state_vars, element, target, dt):
particles.update_natural_coords(element)
if element.constraints:
element.apply_boundary_constraints()
particles.compute_strain(element, dt)
stress = particles.material.compute_stress(None, particles, state_vars)
assert jnp.allclose(stress, target)
@pytest.mark.parametrize(
"particles, element, target",
particles_element_targets,
)
def test_compute_stress(particles, element, target):
state_vars = {"pressure": jnp.zeros(1)}
dt = 1
particles.update_natural_coords(element)
if element.constraints:
element.apply_boundary_constraints()
particles.compute_strain(element, dt)
stress = particles.material.compute_stress(None, particles, state_vars)
assert jnp.allclose(stress, target)

No need to mark them as fixtures if they're just returning a value for one test. Just use them in the test directly.

# Compute the stress
def compute_stress(self, dstrain, particles, state_vars:dict):
"""
Computes the stress for the Bingham material
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Computes the stress for the Bingham material
Computes the stress for the Bingham material.

@SachinJalan SachinJalan marked this pull request as ready for review July 16, 2023 19:05
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