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

Add feature for multiple lattice parameters/phases in polycrystal_from_odf #16

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

vamatya
Copy link
Contributor

@vamatya vamatya commented Aug 29, 2024

Feature add for ingesting multiple lattice parameters, sgname and cif files, and corresponding phase fractions for polycrystal_from_odf function.

Copy link
Collaborator

@AxelHenningsson AxelHenningsson left a comment

Choose a reason for hiding this comment

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

This looks solid to me. :=)

Altough, we need to make the unit tests pass / add one for the case of multi phase, and update the examples.

unit_cell (:obj:`list` of :obj:`float`): Crystal unit cell representation of the form
[a,b,c,alpha,beta,gamma], where alpha,beta and gamma are in units of degrees while
unit_cell (:obj:`list of lists` of :obj:`float`): Crystal unit cell representation of the form
[[a,b,c,alpha,beta,gamma],], where alpha,beta and gamma are in units of degrees while
a,b and c are in units of anstrom.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a sentence to each of these arguments which specify that "When unit_cell is an iterable with several instances of unit cell parameters, the polycrystal represents a multi-phase material."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is updated.

@@ -131,6 +132,8 @@ def polycrystal_from_odf(
strain_tensor(x) -> :obj:`numpy array` of shape ``(3,3)`` where input variable ``x`` is
a :obj:`numpy array` of shape ``(3,)`` representing a spatial coordinate in the
cylinder (x,y,z).
phase_fraction (:obj: `list` of :obj:`float`): Phase fraction represented as probability, summing up to 1.
Default None; will take even distribution of crystals.

Returns:
(:obj:`xrd_simulator.polycrystal.Polycrystal`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also neeed to edit the example file. c.f docs/source/examples/example_polycrystal_from_odf.py

This example is included into the docs so it now needs to reflect the changes. I suggest to make it a simple 2-phase material example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a 2 phase example, albeit with trivial input values.

# Sample is uniformly single phase
phases = [Phase(unit_cell, sgname, path_to_cif_file)]
element_phase_map = np.zeros((mesh.number_of_elements,)).astype(int)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks solid to me. :=)

Altough, we need to make the unit tests pass / add one for the case of multi phase.

@AxelHenningsson
Copy link
Collaborator

Great that you are contributing @vamatya ❤️ 🤟 🎉

Some things still need to be done before we can merge.

  1. Make the unit tests pass - i.e. propagate any changes into the unit tests.
  2. Add a simple unit tests that verify that the new feature works as expected.
  3. Update the examples to reflect the changes - and to showcase your new and awesome feature that you added ! 🚀

Cheers,
Axel

2. Update code to handle both single and multi-phase inputs
3. Add unit test for 2 phase example
4. Add example for 2 phase material
@AxelHenningsson AxelHenningsson merged commit 89461d5 into FABLE-3DXRD:main Sep 17, 2024
1 check 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.

2 participants