-
Notifications
You must be signed in to change notification settings - Fork 27
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
Use phase class #201
Conversation
I'm happy to see that you use One issue is related to the rotation sampling methods, which I made a comment about in the PR which this PR supersedes: #197 (comment). |
@hakonanes how does the |
Okay I've decided a couple of things are kind of anti patterns in I've come to a "One class to rule them all" mentality similar to the
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 That being said the output should be the same (a So that leads us to a
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 |
@hakonanes What exactly is the check-Manifest test (and why is it failing :)) |
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. |
2376554
to
1598b68
Compare
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. |
6c2a48f
to
6679f31
Compare
I'm happy to take on the review of this. |
@pc494 that would be great! I haven't changed the 1 d simulation but that can be done in a different pr |
There was a problem hiding this 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
.
|
||
def __init__( | ||
self, | ||
since: Union[str, int, float], |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
@hakonanes any chance you can look over the Phase implementation here? |
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...). |
@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. |
sorry @hakonanes, this one is on me... |
As in you'll review it? Perfect! |
There was a problem hiding this 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? :)
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. |
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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.
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 I'll make a migration guide.
Good point! I'll crawl through the documentation to make sure that is there
Will do!
Yea! Let's see what I can do this weekend to clean up all of this stuff! |
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. |
Fix test failures
87930d5
to
6840707
Compare
Does #205 mean we can close this @CSSFrancis? |
Close in favor of #205 |
Description of the change
This changes the simulations to use the
orix.crystal_map.Phase
class and to use theReciporicalLatticeVector
for most everything.It also changes how simulations are set up so that some
Phase
andRotation
can be passed to theSimulationGenerator
object and a list of simulations are returned.Progress of the PR
SimulationLibrary
SimulationLibrary
SimulationLibrary.Rotation
- [ ] Add plot method forSimulationLibrary
(to Hyperspy for visualization?)Phase
Phase
objectsMinimal example of the bug fix or new feature
This supercedes #197
For reviewers
__init__.py
.unreleased section in
CHANGELOG.rst
.credits
indiffsims/release_info.py
andin
.zenodo.json
.