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

GetStrain function #138

Merged
merged 16 commits into from
Aug 16, 2022
Merged

GetStrain function #138

merged 16 commits into from
Aug 16, 2022

Conversation

f-iniv
Copy link
Contributor

@f-iniv f-iniv commented Aug 4, 2022

Function to obtain strain matrices from a desired stress matrix using the elastic tensors from a crystal

Added a function to convert from the elastic to the Compliance tensor, and definitions for monoclinc and trigonal elastic tensors

@dkriegner
Copy link
Owner

Thanks for the contribution. this looks interesting.

I would propose few changes before I am willing to merge it tough. In particular I think the elastic parameter of GetStrain should be either dropped or made optional. The Crystal object already should possess all the required elastic parameters in the cij property. Could you check this?

Also I think it would be good to conceive some unit tests to continuously test that this function gives sensible results. Would you be able to provide a unit test for this new function?

@f-iniv
Copy link
Contributor Author

f-iniv commented Aug 5, 2022

I made the cij property as the standard, removed elastic, changed sig as a matricial input to match ApplyStrain and also added unit tests for the stress and elastic tensors.

If you think anything else needs change, just let me know.

@dkriegner
Copy link
Owner

thank you for the additional work.

I have few more comments:

  1. I think the docstrings of GetStrain and ApplyStrain should mention that the other method exists so that users can find the link between those to related functions easier
  2. If I am not mistaken, the implementation of GetStrain could be much simpler (and faster) using einsum. I think the loops can be avoided by this function. Can you look into this?
  3. Regarding the unittests (although your changes are good) I was actually thinking more of test cases like those implemented in https://github.com/dkriegner/xrayutilities/tree/main/tests. Can you think of something one could include there to ensure this functions (keep) work(ing) correctly? I like to include some tests because they can easily reveal mistakes in future code changes.

'GetStress', added unittests for both "Get" functions
@f-iniv
Copy link
Contributor Author

f-iniv commented Aug 8, 2022

Indeed using einsum was way simpler, it all ended up becoming a single line. I also reduced the size of 'Cij2Sijkl' by using 'Cij2Cijkl' as part of it.

For the unittests, the only way I thought of implementing them was a sort of recursive way, by creating a 'GetStress' function, testing one against the other and having a test for both.

@dkriegner
Copy link
Owner

Thanks for the improvements. The implementation is now cleaner.
I unfortunately still have more comments, but they are getting smaller:

The GetStrain and GetStress functions should likely be methods of Material. Since in Material also all cij matrix is already defined. (Also in Python the self argument is common only for methods of a class). Or do you see any reason this needs to be a normal function?

Do you think the docstrings should mention the units which should be used for the stress? Of course it depends on the units of cij which now we try to use consistently as N/m2.

Also could you add your name to the copyright statement at the beginning of the files you changed? (I will add a note on that to the contributing file in the main folder)

@f-iniv
Copy link
Contributor Author

f-iniv commented Aug 15, 2022

No worries!

The way I was using it previously was a function, but there's no reason for it not to be a method, so I already fixed it.

I also agree on mentioning the units used for the stress, initially I intended to add that, but eventually forgot.

@dkriegner dkriegner merged commit 70d9075 into dkriegner:main Aug 16, 2022
@dkriegner
Copy link
Owner

thanks again for the work on this.
If you have other ideas what to change feel free to submit more PRs!
your addition made me also realize that there are some shortcomings in the Poisson ratio (and other mechanical parameters) implemented in Material. I think these are not used anywhere else in the code, but I guess one should get them correctly from the full anisotropic elasticity tensor. (see e.g. issue #139)

dkriegner added a commit that referenced this pull request Aug 16, 2022
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.

2 participants