Skip to content

ENH Map pixel indices to physical coordinates pyFAI detector#274

Draft
LouConreux wants to merge 3 commits intoslac-lcls:masterfrom
LouConreux:ENH/pyfai_azint
Draft

ENH Map pixel indices to physical coordinates pyFAI detector#274
LouConreux wants to merge 3 commits intoslac-lcls:masterfrom
LouConreux:ENH/pyfai_azint

Conversation

@LouConreux
Copy link
Copy Markdown

@LouConreux LouConreux commented Feb 10, 2026

Description

This PR aims at improving the pyFAI azimuthal integration script. Rather than assembling the images at each event, we define a mapping between pixel and physical space only once through the use of LCLSGeom creating a pyFAI detector object with pixel coordinates. This discards the need for casting 3D images into hard-coded 2D assembled images.

Checklist

  • Add LCLSGeom to path/environment (see Address Issues)
  • Modifications to smalldata_tools/ana_funcs/azav_pyfai.py

PR Type

  • Enhancement: provide better and faster pyFAI azimuthal integration pipeline

Address Issues

  • How to import LCLSGeom at top of script
    • Pass it through LUTE ?
    • Add to environment ?

Test on LCLS-II detectors

  • Test was done through lute on mfx101343025 run 8 at /sdf/data/lcls/ds/mfx/mfx101343025/results/lconreux/launchpad/pyfai_ENH for logs.

  • Command run:

  1. Source /sdf/data/lcls/ds/mfx/mfx101343025/results/lconreux/lute/install/bin/activate_installation
  2. Run:
    submit_slurm -t SmallDataProducer2 -c /sdf/data/lcls/ds/mfx/mfx101343025/results/lconreux/yamls/smd_pyfai_config_1.yaml -e mfx101343025 -r 8 --psana2 --partition=milano --account=lcls:prjlute22 --nodes=8 --ntasks-per-node=50 --exclusive
  • Output logs /sdf/data/lcls/ds/mfx/mfx101343025/results/lconreux/launchpad/pyfai_ENH/SmallDataProducer2Test_mfx101343025_r0008_2026-03-11_18-27-45_23131031.out:
INFO:lute.execution.executor:Joining: 8 files --> mfx101343025_Run0008.h5

INFO:lute.execution.executor:([ 2026-03-11 11:37:23,557 | INFO | smd_producer.py] Getting epics data from Archiver (rank: 9)
)
INFO:lute.execution.executor:########## JOB TIME: 9.098399 minutes ###########
Last Event: 270

INFO:lute.execution.executor:
ERROR:lute.io.elog:eLog Update Failed! JID_UPDATE_COUNTERS is not defined!
ERROR:lute.io._db.v2.api:Cannot setup permissions on database!
INFO:lute.execution.executor:Exiting after Task completion.

… detector object, rather than at every event assembling the image
@LouConreux LouConreux marked this pull request as ready for review February 10, 2026 22:00
@LouConreux LouConreux marked this pull request as draft February 10, 2026 22:00
@gadorlhiac
Copy link
Copy Markdown
Collaborator

The code change itself looks fine to me, but the path issues are what need to be resolved - and this repo itself is probably not the place where we can make that decision.

Regardless of the ultimate decision, I don't think we should do the path manipulation to make this available - it will be very brittle.

Some options - number 1 is by far the easiest for many use cases:

  1. Adding to the environment, as has been discussed many times before - I don't recall what the issue was?
  2. Using the "central" install version (could be added to the bash script?).
  3. When run inside LUTE there are already multiple ways to address this. But not everyone is adopting that yet.
  • To add even another, I have no issue vendoring your software as a small dependency for LUTE in the interim. I.e. every installation of LUTE would bring it along (would end up in the install dir for example). It won't work with the build CI because you need a PyPI package, but local installs using build.sh would be fine.

@LouConreux
Copy link
Copy Markdown
Author

It was not really an issue, I just realized I needed to clean the code before adding LCLSGeom to the environment.
Now, I think it looks production-ready. I think putting it in the environment would be the easiest since lute and maybe smalldata will use it.

@gadorlhiac
Copy link
Copy Markdown
Collaborator

Yup I agree - if it can be added to the environment it should be 👍

Just with regards to this PR it should be added before merging so the path modifications can be taken out

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.

2 participants