-
Notifications
You must be signed in to change notification settings - Fork 27
Bugfix: Small Bugfix for polar flatten simulation #238
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
Conversation
f104075 to
6eda489
Compare
5c5af32 to
8aab105
Compare
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.
Pull Request Overview
This PR addresses a bug in the polar flatten simulation by updating the test expectations and refining the masking in the polar flatten conversion. Key changes include:
- Updating the test cases in Simulation2D to expect 8 spots instead of 1.
- Changing the hkls format in Simulation1D tests from string representations to numeric arrays.
- Refining the masking logic in polar flatten simulations and updating dependency versions.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| diffsims/tests/simulations/test_simulations2d.py | Updated test assertions to expect 8 spots and adjusted radial_axes values. |
| diffsims/tests/simulations/test_simulations1d.py | Changed hkl representation from string to numeric array. |
| diffsims/tests/generators/test_simulation_generator.py | Removed string-specific handling for hkls and updated progressbar assertions. |
| diffsims/simulations/simulation2d.py | Revised the masking logic in polar flatten simulations. |
| diffsims/simulations/simulation1d.py | Expanded plot function parameters and updated label formatting. |
| diffsims/generators/simulation_generator.py | Adjusted reciprocal lattice vector processing in 1D simulation. |
| .github/workflows/build.yml | Updated build dependencies. |
Comments suppressed due to low confidence (2)
diffsims/tests/simulations/test_simulations2d.py:107
- The test now expects 8 spots per simulation rather than 1. Please double-check that the polar flatten simulation logic correctly maps the new coordinate length to the output arrays.
assert r_templates.shape == (1, 8)
diffsims/simulations/simulation2d.py:330
- [nitpick] Consider renaming the variable 'inten' to 'intensity' for clarity and consistency with the attribute name.
inten = v.intensity
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
@wiscott24 Does this fix your problem? |
viljarjf
left a comment
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.
Seems good! There might be some merit to giving a warning if the phase is not symmetrically expanded, as the structure factors will not be correct otherwise. just something simple like
if len(phase.structure) != len(phase.expand_asymmetric_unit().structure):
warnings.warn(...)Otherwise this looks fine to me! I have made some changes to calculate_simulation2d in another draft PR, I can see if the same can be applied to simulation1d when I get around to finalizing it.
Yes, thanks! |
Use regex to allow multiple lengths for progress bar
|
@viljarjf hmmm. Should we just have something like: if len(phase.structure) != len(phase.expand_asymmetric_unit().structure):
phase = phase.expand_asymmetric_unit() |
I'm not really sure. Users might want to only have certain atoms, to see how that might affect things? If so, they can use P1 I guess. On the other hand, I expect most users use a cif, in which case all atoms are present already. Displaying a warning is also not done in many places, outside of maybe orix warning you if it thinks you meant to use I say we can leave it for now, maybe move it to a seperate issue for further discussion. This is also a potential problem in 2D, ref #234 |
Description of the change
It seems like we weren't doing a great job of masking values etc which when doing a orientation map could have some pretty big impacts.
We should make a bug patch release probably asap.
Progress of the PR