Skip to content

Conversation

@CSSFrancis
Copy link
Member

@CSSFrancis CSSFrancis commented May 30, 2025

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

@CSSFrancis CSSFrancis force-pushed the bugfix_simulation branch from 5c5af32 to 8aab105 Compare May 30, 2025 19:56
@CSSFrancis CSSFrancis requested a review from Copilot May 30, 2025 20:19
Copy link
Contributor

Copilot AI left a 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>
@CSSFrancis
Copy link
Member Author

pre-commit.ci autofix

@CSSFrancis
Copy link
Member Author

@wiscott24 Does this fix your problem?

@CSSFrancis
Copy link
Member Author

@viljarjf any chance that you can look at this? This fixes the failing tests with #235. Sorry it took me so long to get around to that. Also feel free to just ping me for stuff if I'm being slow. Sometimes I see things and mean to go back to it and forget :)

Copy link
Contributor

@viljarjf viljarjf left a 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.

@wiscott24
Copy link

@wiscott24 Does this fix your problem?

Yes, thanks!

Use regex to allow multiple lengths for progress bar
@CSSFrancis
Copy link
Member Author

@viljarjf hmmm.

Should we just have something like:

if len(phase.structure) != len(phase.expand_asymmetric_unit().structure):
    phase = phase.expand_asymmetric_unit()

@viljarjf
Copy link
Contributor

viljarjf commented Jun 2, 2025

@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 degrees=True, so this might seem out of place.

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

@CSSFrancis CSSFrancis merged commit 347f7d4 into pyxem:main Jun 2, 2025
17 checks 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.

3 participants