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

Use phase class #201

Closed
wants to merge 48 commits into from
Closed

Use phase class #201

wants to merge 48 commits into from

Conversation

CSSFrancis
Copy link
Member

@CSSFrancis CSSFrancis commented Oct 25, 2023

Description of the change

This changes the simulations to use the orix.crystal_map.Phase class and to use the ReciporicalLatticeVector for most everything.

It also changes how simulations are set up so that some Phase and Rotation can be passed to the SimulationGenerator object and a list of simulations are returned.

Progress of the PR

  • Add SimulationLibrary
    • Add Test for SimulationLibrary
    • Add plot method for SimulationLibrary.Rotation
      - [ ] Add plot method for SimulationLibrary (to Hyperspy for visualization?)
  • Add SimulationLibraries
    • Add good repr
    • Add Tests
  • Add SimulationGenerator
  • Add Examples
    • Add example for simulating one diffraction pattern
    • Add example for simulating multiple diffraction patterns from one Phase
    • Add example for simulating multiple diffraction patterns from multiple Phase objects
  • Deprecation
    • Deprecate DiffractionGenerator
    • Deprecate LibraryGenerator
    • Deprecate StrucutureLibrary
    • Deprecate StrucutureLibraryGenerator
  • Docstrings for all functions
  • Unit tests with pytest for all lines
  • Clean code style by running black

Minimal example of the bug fix or new feature

####################################
# Simulating One Diffraction Pattern
# ==================================

from orix.crystal_map import Phase
from orix.quaternion import Rotation
from diffpy.structure import Atom, Lattice, Structure

from diffsims.generators.simulation_generator import SimulationGenerator

a = 5.431
latt = Lattice(a, a, a, 90, 90, 90)
atom_list = []
for coords in [[0, 0, 0], [0.5, 0, 0.5], [0, 0.5, 0.5], [0.5, 0.5, 0]]:
    x, y, z = coords[0], coords[1], coords[2]
    atom_list.append(Atom(atype="Si", xyz=[x, y, z], lattice=latt))  # Motif part A
    atom_list.append(Atom(
            atype="Si", xyz=[x + 0.25, y + 0.25, z + 0.25], lattice=latt
        )
    )  # Motif part B
struct = Structure(atoms=atom_list, lattice=latt)
p = Phase(structure=struct, space_group=227)

gen = SimulationGenerator(accelerating_voltage=200,)
rot = Rotation.from_axes_angles([1, 0, 0], 45, degrees=True)  # 45 degree rotation around x-axis
sim = gen.calculate_ed_data(phase=p, rotation=rot)

sim.simulations[0].plot()  # plot the first (and only) diffraction pattern

#%%

This supercedes #197

For reviewers

  • The PR title is short, concise, and will make sense 1 year later.
  • New functions are imported in corresponding __init__.py.
  • New features, API changes, and deprecations are mentioned in the
    unreleased section in CHANGELOG.rst.
  • Contributor(s) are listed correctly in credits in diffsims/release_info.py and
    in .zenodo.json.

@CSSFrancis CSSFrancis marked this pull request as draft October 25, 2023 14:20
@CSSFrancis CSSFrancis mentioned this pull request Oct 25, 2023
16 tasks
@hakonanes
Copy link
Member

I'm happy to see that you use Phase and Rotation directly! These classes should have the necessary visualization tools required for validating the phase and rotation list. Please let me know if you have any questions in this regard. I'm more than happy to help with changes to these classes that make sense in the context of orix.

One issue is related to the rotation sampling methods, which I made a comment about in the PR which this PR supersedes: #197 (comment).

@CSSFrancis
Copy link
Member Author

I'm happy to see that you use Phase and Rotation directly! These classes should have the necessary visualization tools required for validating the phase and rotation list. Please let me know if you have any questions in this regard. I'm more than happy to help with changes to these classes that make sense in the context of orix.

@hakonanes how does the Rotation visualization work? I've been trying to figure that out but can't seem to visualize the different rotations. Can I just force them to be Vector3D object somehow and then plot the stereographic projection from there?

@CSSFrancis
Copy link
Member Author

CSSFrancis commented Oct 28, 2023

Okay I've decided a couple of things are kind of anti patterns in diffsims and I don't really like the shear number of classes/ generators etc.

I've come to a "One class to rule them all" mentality similar to the BaseSignal class in hyperspy. There are really 3 different situations we want to handle.

  1. One Phase and one Rotation
  2. One Phase and multiple Rotation(s)
  3. Multiple Phase(s) and multiple Rotation(s)

All of them are useful and we would like to be able to template match vs all of these options for

1--> Orientation mapping on some zone axis with a tilt boundary
2 --> Orientation mapping on a crystal with multiple orientations but a single phase
3 --> Orientation mapping on a crystal with multiple orientations and multiple phases

That being said the output should be the same (a CrystalMap object). Further there really isn't need for more than one generator object (a DiffractionGenerator)

So that leads us to a Simulation object which has 3 levels of organization.

phases --> rotations -->coordinates where each phase has some rotations and each rotation has some coordinates.

Let's see what that means:

sim # simulation
sim.iphase["al"] # Returns only the simulation of the Al phase
sim.irot[0:4] # Returns only the first 4 rotations for all phases

sim.plot() # plots the simulation at sim.phase_index, sim.rot_index

So you can do things like...

sim.iphase["al"].irot[2].plot()

It also gives us the potential to add in things like interactive plotting if we want to without much extra work. The fact that everything is the same object gives us lots of flexibility when it comes to creating simulations, slicing them etc. It also makes it possible for them to grow a little bit and we could include some more complexity/ extend them as well.

@hakonanes any thoughts? I have a working class in this PR under diffsims.simulations.Simulation

@CSSFrancis
Copy link
Member Author

@hakonanes What exactly is the check-Manifest test (and why is it failing :))

@hakonanes
Copy link
Member

hakonanes commented Nov 15, 2023

It matches all files listed in MANIFEST.in intended for inclusion in the source distribution to an actual distribution it creates. It fails if there is a mismatch.

The MANIFEST.in file ensures that the source dist contains just what we intend and nothing else.

@CSSFrancis
Copy link
Member Author

Just for reference we should be able to use something like hyperspy/hyperspy#3271 for visualization. I think that would be better than trying to reimplement the visualization tools here.

@CSSFrancis CSSFrancis force-pushed the use_phase_class branch 2 times, most recently from 6c2a48f to 6679f31 Compare November 29, 2023 21:57
@CSSFrancis CSSFrancis marked this pull request as ready for review November 30, 2023 20:50
@pc494 pc494 self-requested a review December 1, 2023 18:17
@pc494
Copy link
Member

pc494 commented Dec 1, 2023

I'm happy to take on the review of this.

@CSSFrancis
Copy link
Member Author

@pc494 that would be great! I haven't changed the 1 d simulation but that can be done in a different pr

Copy link
Member

@pc494 pc494 left a comment

Choose a reason for hiding this comment

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

This is generally a great PR making a much-needed change. It does still need a bit of cleaning up though.

If @hakonanes has the capacity to look into some of the more technical bits of the implementation, I didn't spend that much time working directly with Phase.

CHANGELOG.rst Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
diffsims/simulations/simulation.py Outdated Show resolved Hide resolved
diffsims/simulations/simulation.py Outdated Show resolved Hide resolved
diffsims/simulations/__init__.py Outdated Show resolved Hide resolved
diffsims/tests/simulations/test_simulation.py Show resolved Hide resolved

def __init__(
self,
since: Union[str, int, float],
Copy link
Member

Choose a reason for hiding this comment

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

this feels to generous as a typing. Surely string is enough?

# ============================================================
# Simulating Multiple Rotations for Multiple Phases
# ============================================================
p2 = p.deepcopy()
Copy link
Member

Choose a reason for hiding this comment

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

might be worth a comment about the use of .deepcopy() here?

@CSSFrancis
Copy link
Member Author

@hakonanes any chance you can look over the Phase implementation here?

@hakonanes
Copy link
Member

I'll review it this weekend. Thank you for pinging me. And your patience (who'd have thought that a new job, relocating, and PhD defence coming up would mean such busy times...).

@CSSFrancis
Copy link
Member Author

@hakonanes No problem, thanks for reviewing this!

I'm probably 3 months behind you, so I'm going to be there very very soon. Just trying to finish some stuff up before I'm done.

@pc494
Copy link
Member

pc494 commented Jan 10, 2024

sorry @hakonanes, this one is on me...

@CSSFrancis CSSFrancis mentioned this pull request Jan 10, 2024
2 tasks
@hakonanes
Copy link
Member

As in you'll review it? Perfect!

Copy link
Member

@pc494 pc494 left a comment

Choose a reason for hiding this comment

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

This is more or less good to go. @CSSFrancis while you were building this, were there consistency checks that the results agreed with what we had been getting previously? If so might be worth documenting them in the PR. That would leave

  • Coax the final bits of documentation into agreement with standards.
  • Provide some evidence of sensible simulations (doesn't have to be loads).

Then I reckon it's merge and make a new minor release time? :)

diffsims/generators/simulation_generator.py Show resolved Hide resolved
Comment on lines 114 to 164
phase:
The phase(s) for which to derive the diffraction pattern.
reciprocal_radius
The maximum radius of the sphere of reciprocal space to
sample, in reciprocal Angstroms.
rotation
The Rotation object(s) to apply to the structure and then
calculate the diffraction pattern.
with_direct_beam
If True, the direct beam is included in the simulated
diffraction pattern. If False, it is not.
max_excitation_error
The cut-off for geometric excitation error in the z-direction
in units of reciprocal Angstroms. Spots with a larger distance
from the Ewald sphere are removed from the pattern.
Related to the extinction distance and roungly equal to 1/thickness.
shape_factor_width
Determines the width of the reciprocal rel-rod, for fine-grained
control. If not set will be set equal to max_excitation_error.
debye_waller_factors
Maps element names to their temperature-dependent Debye-Waller factors.
Copy link
Member

Choose a reason for hiding this comment

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

This also should have types as the typing in the args is afaik optional.

Comment on lines +218 to +260
The phase for which to derive the diffraction pattern.
reciprocal_radius
The maximum radius of the sphere of reciprocal space to
sample, in reciprocal Angstroms.
minimum_intensity
The minimum intensity of a reflection to be included in the profile.
debye_waller_factors
Maps element names to their temperature-dependent Debye-Waller factors.
Copy link
Member

Choose a reason for hiding this comment

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

see above comments about docstrings.

@CSSFrancis
Copy link
Member Author

This is more or less good to go. @CSSFrancis while you were building this, were there consistency checks that the results agreed with what we had been getting previously? If so might be worth documenting them in the PR. That would leave

Yea let me add those in. I realized last night that there are some (small) inconstancies which arose because we aren't calculating the zero beam I think the Vector3D class in orix doesn't love the idea of a [0,0,0] vector. The result is that the thresholding doesn't work properly. This is kind of like the old method with_zero_beam=False

I'll make a migration guide.

  • Coax the final bits of documentation into agreement with standards.

Good point! I'll crawl through the documentation to make sure that is there

  • Provide some evidence of sensible simulations (doesn't have to be loads).

Will do!

Then I reckon it's merge and make a new minor release time? :)

Yea! Let's see what I can do this weekend to clean up all of this stuff!

@hakonanes
Copy link
Member

BTW, orix forces a crystal axes alignment that is different from diffpy.structure. Simulations before and after this PR of a crystal with non-right angles, such as a triclinic or monoclinic crystal, should be compared.

@pc494
Copy link
Member

pc494 commented Apr 30, 2024

Does #205 mean we can close this @CSSFrancis?

@CSSFrancis
Copy link
Member Author

Close in favor of #205

@CSSFrancis CSSFrancis closed this Apr 30, 2024
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.

None yet

5 participants