Skip to content

Conversation

gudlot
Copy link
Collaborator

@gudlot gudlot commented Jul 7, 2025

This PR is a work in progress for adding reflection support as discussed in #2555.

Feedback is welcome during development. I’ll mark as “Ready for review” when it's finished.

@gudlot gudlot marked this pull request as ready for review July 7, 2025 13:48
@gudlot gudlot changed the title Reflection support for issue #2555 WIP: Reflection support for issue #2555 Jul 7, 2025
@gudlot gudlot marked this pull request as draft July 7, 2025 13:51
@kif kif self-requested a review July 7, 2025 15:25
@kif kif added the work in progress Don't review label Jul 7, 2025
@kif kif changed the title WIP: Reflection support for issue #2555 [Crystallography] Extinction rules for differents space groups Jul 7, 2025
@kif kif linked an issue Jul 7, 2025 that may be closed by this pull request
@gudlot
Copy link
Collaborator Author

gudlot commented Jul 8, 2025

Hi @kif,

There was something fishy going on. The tests didn’t fail even when I deliberately broke the code for a space group. I looked into test_calibrant.py and found this output with both unittest and pytest:

setUpClass (pyFAI.test.test_calibrant.TestReflection) ... skipped 'network unreachable.'
src/pyFAI/test/test_calibrant.py::TestReflection::test_code SKIPPED (network unreachable.)

The class TestReflection is defined, and so is its method test_code, but the test is skipped. From what I can tell, this happens because UtilsTest.getimage("reflection.h5") doesn't find the file. There’s no explicit SkipTest raised — it seems like the skip is triggered internally or indirectly by getimage().

Right below the TestReflection class, there’s a large commented-out block that would generate reflection.h5 using xrayutilities.

  • Is it intended that this test runs only if that file already exists?
  • Should the file be regenerated or tracked in the repo?

Right now, the test is silently skipped and doesn't warn about a missing critical file. This makes it easy to miss bugs — I only noticed it because I broke the logic for one of the space groups on purpose.

Update: I was able to re-create reflection.h5 locally. Now the test fails as expected, when the space group is wrongly implemented.

@kif
Copy link
Member

kif commented Jul 8, 2025

Well the file is supposed to be cached in the /tmp directory:
os.listdir(pyFAI.test.utilstest.UtilsTest.resources.data_home)

Those files are all available on:
http://www.silx.org/pub/pyFAI/testimages/

I have very often tests failing in the CI because of files that could not be downloaded, there is a mechanism that ensures the checksum of the file is correct (which allows to upgrade the file when needed, enforcing the testing machine to re-download the file) ...

Moreover, there is nothing related to the skipping of tests in https://github.com/silx-kit/silx/blob/main/src/silx/utils/ExternalResources.py

I don't know if this is a feature introduced by pytest ... but it could be an extra reason not using it :trollface:

I just checked, with the file missing, the test takes 300ms (including the download) while the second run takes only 80ms.

@gudlot
Copy link
Collaborator Author

gudlot commented Jul 8, 2025

Thank you very much for the explanation and your help, @kif.
I observed earlier the same behavior when using unittest:

setUpClass (pyFAI.test.test_calibrant.TestReflection) ... skipped 'network unreachable.'

So maybe it doesn’t seem to be related to pytest specifically.
Interestingly, it happened on two different networks on my end, so perhaps just bad timing or something transient.

For reference, my cached file is located at:
/var/folders/hy/lpgvb6d920s1g6_knj5sryc00000gn/T/pyFAI_testdata_gudrun/reflection.h5
Thanks again for clarifying—and sorry for raising the point if it was already well understood.

@kif
Copy link
Member

kif commented Jul 9, 2025

Hi Gudrun,
Your code looks correct to me, I just checked in the international tables. BTW, I realized those tables are not freely available on the internet, as they are from inside ESRF.
This SG deserves specific investigation, could be there is an issue in xrayutilities or elsewhere.
Thanks for the huge job you already achieved.
Jerome

Copy link
Member

@kif kif left a comment

Choose a reason for hiding this comment

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

Very good documentation.

@gudlot
Copy link
Collaborator Author

gudlot commented Jul 16, 2025

Next Space Groups Implemented

Hi @kif

Here are the next space groups. I mostly followed the ITC definitions.
In cases where the xrayutilities tests failed, I consulted the rules listed at http://img.chem.ucl.ac.uk/sgp/large/sgp.htm and adapted the implementations accordingly.
With those adjustments, the tests against xrayutilities passed.

SG 110 was particularly tricky. If you have time, maybe you could take a look at it later.
Currently, all tests pass.

Best,
Gudrun

@gudlot
Copy link
Collaborator Author

gudlot commented Jul 22, 2025

Hi @kif
I observed a problem with SG 161. I followed the rules in ITC p533, but the code implementation fails against xrayutilities. I commented out code that would not fail the the test, but I cannot find a physical justification for it. Hence, I am reluctant to use it. Maybe you could help me out here, please?
Many thanks.
Best regards, Gudrun

@gudlot
Copy link
Collaborator Author

gudlot commented Jul 29, 2025

I investigated further.
xrayutilities reports the following for space group 167:

sg 167:H trigonal:H R-3c:H: a = 1.1000, b = 1.1000 c= 1.2000
alpha = 90.000, beta = 90.000, gamma = 120.000
Reflection conditions:
 general: hkil: -h+k+l=3n, hki0: -h+k=3n, hh(-2h)l: l=3n, h-h0l: h+l=3n, l=2n, 000l: l=6n, h-h00: h=3n

Evaluating the above for the given set of indices:

(False, {(1, 0, -5), (-1, 0, 5), (-1, 1, -5), (1, -1, 5), (0, 1, 5), (0, -1, -5)})

Maybe there is another rule, I am not yet aware of.

@gudlot
Copy link
Collaborator Author

gudlot commented Jul 30, 2025

I found the solutions for SG167, SG161, on offer two solutions for SG167. All tests green AFAIK.

@gudlot gudlot force-pushed the 2555_gudlot_reflection branch from 0b8bc0d to 5da256c Compare August 5, 2025 09:48
@kif kif added ready to merge Please review and removed work in progress Don't review labels Sep 24, 2025
Copy link
Member

@kif kif left a comment

Choose a reason for hiding this comment

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

Impressive work. Looks great !

@kif kif marked this pull request as ready for review September 24, 2025 19:43
@kif kif merged commit 32a39f9 into silx-kit:main Sep 24, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Please review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Crystallography]: Validate all 230 space groups for extinction rules
2 participants