IMP: Add Epix10k support #42
Comments
|
cc @chrisvam |
|
@tjlane I'll give it a go, let me look at the code |
|
awesome! post here any time with questions, and we can zoom/email too if useful |
|
Last thing we need to do: update
One thing we can do to help the basic user: try and inspect crystFEL geometries for strings matching the above and use them to automatically interpret the geometry. Alternatively we could try and match the expected shapes of the detectors. Thoughts @valmar ? |
|
Rather than guessing, I prefer to ask the user. I would go with the flag. I will implement thar
Valerio
…-------- Original Message --------
On Jun 25, 2020, 11:42 AM, TJ Lane wrote:
Last thing we need to do: update geoconv to take a flag indicating what kind of sensor to use when interpreting a geometry file. Included should be:
- CSPAD
- Epix10ka
- Jungfrau
One thing we can do to help the basic user: try and inspect crystFEL geometries for strings matching the above and use them to automatically interpret the geometry. Alternatively we could try and match the expected shapes of the detectors.
Thoughts ***@***.***(https://github.com/valmar) ?
—
You are receiving this because you were mentioned.
Reply to this email directly, [view it on GitHub](#42 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAEILBIMSBQTCZAVAWBOLV3RYMLYJANCNFSM4N73LKZA).
|
|
Seems reasonable. We definitely want that option anyways, so let's go for that :). Let me know if you need help. |
|
@tjlane should we go for -s,--sensor. Also, how important is back-compatibility? Should we keep around the --cspad switch, or do we incorporate that in the --sensor one? |
|
Maybe slight preference for I don't think backward compatibility is a major issue. I gave Chris a heads up that the next version would be "a major change" and may break things. So my vote would be to keep things simple: just have the one general flag and remove the |
|
Perfect, I'll do that, thank you
…-------- Original Message --------
On Jun 25, 2020, 6:31 PM, TJ Lane wrote:
Maybe slight preference for -s and --sensor-type since that may be a little clearer for the user. But a nice help string is probably the most important.
I don't think backward compatibility is a major issue. I gave Chris a heads up that the next version would be "a major change" and may break things. So my vote would be to keep things simple: just have the one general flag and remove the --cspad switch.
—
You are receiving this because you were mentioned.
Reply to this email directly, [view it on GitHub](#42 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAEILBM4ENGWPLCFEXHATTTRYN3U3ANCNFSM4N73LKZA).
|
|
@tjlane Sorry one more question, which might also be related to #41 Currently geoconv calls "load" passing as base "Cspad" or nothing. How do you envision the solution to this issue in code (I will then code it)? Should we define several "bases" in the "camera" module in addition to Cspad? The only thing that I see the Cspad class doing differently is that it groups the panels into quadrants as it loads them. Should we do the same for expix10ka and Jungfrau? Speaking of which, should we have, for example, a different class for every Epix10KA detector, like: "Epix10ka2m", "epix10ka4m", etc? (Otherwise, how do we know how the quadrants are split)? |
|
Good question. The work I've been doing over the past few months on psgeom has aimed to make these detector-specific classes obsolete, aiming for generality. Previously the CSPAD was basically coded as an exception, but that's (almost) not necessary anymore. Since the number of detector types is increasing, I don't think we should try to keep up with treating each of them as a separate thing. So the new design paradigm is: we have to specify what the rigid "sensor" looks like (as you did for the Epix10ka), but for any detector made up of a combination of those sensors, the rest should be generic. As an example you can see how the CSPAD 2M and CSPAD 2x2 are handled: basically psgeom doesn't care how many CSPAD 2x1 units form a detector. So for psana crystfel Loading psana is no issue, as we can always interpret the correct sensor type. When we load a crystfel geometry, however, we need to get the sensor type from the user. But then we can just set it using the Some time in the future I hope to completely remove |
|
Ok, but like this we will have a camera.CompoundAreaCamera object with just a set of sensors, so one layer of abstraction. How do we deal with intermediate ones like quadrants, for example for the Epix10KA? I bombard you with questions because I want this to be crystal clear, so that I can mantain the package when you are busy |
|
No problem at all with questions. We can also zoom at some point if it ever is necessary. Until then keep them coming! Also I will help maintain things when I can, but appreciate your help :). There is a little doc string here that explains this a bit: Working on documentation is definitely a to-do. In short: any group of sensors can form a So, for the CSPAD 2M or Epix10ka, each quad is a To see the hierarchy of any detector, you can do something like this (where I am using our example Epix10ka with 4 quads): In [1]: from psgeom import camera
In [2]: geo = camera.CompoundAreaCamera.from_psana_file('540-541.data')
Loading: 540-541.data
In [3]: geo.draw_tree()
--- CAMERA-0
|-- QUAD-0
|-- EPIX10KA:V1-0
|-- EPIX10KA:V1-1
|-- EPIX10KA:V1-2
|-- EPIX10KA:V1-3
|-- QUAD-1
|-- EPIX10KA:V1-0
|-- EPIX10KA:V1-1
|-- EPIX10KA:V1-2
|-- EPIX10KA:V1-3
|-- QUAD-2
|-- EPIX10KA:V1-0
|-- EPIX10KA:V1-1
|-- EPIX10KA:V1-2
|-- EPIX10KA:V1-3
|-- QUAD-3
|-- EPIX10KA:V1-0
|-- EPIX10KA:V1-1
|-- EPIX10KA:V1-2
|-- EPIX10KA:V1-3The quad definitions & hierarchy come from the psana geometry file format. |
|
OK that is clearer now, but then for the crystfel -> psana conversion, where will the quadrant information come from? If we hardcode it, we go back to the detector specific code. Should we already implement the rigid group reading code
…-------- Original Message --------
On Jun 26, 2020, 7:40 PM, TJ Lane wrote:
No problem at all with questions. We can also zoom at some point if it ever is necessary. Until then keep them coming! Also I will help maintain things when I can, but appreciate your help :).
There is a little doc string here that explains this a bit:
https://github.com/slaclab/psgeom/blob/master/psgeom/camera.py#L4
Working on documentation is definitely a to-do.
In short: any group of sensors can form a CompoundCamera (or CompoundAreaCamera, which is just a CompoundCamera restricted to rectangular pixels and planar sensors). But CompoundCameras can then be composed of other CompoundCameras.
So, for the CSPAD 2M or Epix10ka, each quad is a CompoundAreaCamera and the final detector (made of 4 quads) is also a CompoundAreaCamera. This keeps things simple code wise, but allows arbitrary hierarchies. In principle we could do even more complicated shit, like have a CSPAD2x1 mounted on a rigid frame that also holds a CSPAD 2M. Even though one is a "sensor" and one is a "camera" the hierarchical concept works. Hope that is clear.
To see the hierarchy of any detector, you can do something like this (where I am using our example Epix10ka with 4 quads):
In
[
1
]:
from
psgeom
import
camera
In
[
2
]:
geo
=
camera
.
CompoundAreaCamera
.
from_psana_file
(
'540-541.data'
)
Loading
:
540
-
541.
data
In
[
3
]:
geo
.
draw_tree
()
-
-
-
CAMERA
-
0
|
-
-
QUAD
-
0
|
-
-
EPIX10KA
:
V1
-
0
|
-
-
EPIX10KA
:
V1
-
1
|
-
-
EPIX10KA
:
V1
-
2
|
-
-
EPIX10KA
:
V1
-
3
|
-
-
QUAD
-
1
|
-
-
EPIX10KA
:
V1
-
0
|
-
-
EPIX10KA
:
V1
-
1
|
-
-
EPIX10KA
:
V1
-
2
|
-
-
EPIX10KA
:
V1
-
3
|
-
-
QUAD
-
2
|
-
-
EPIX10KA
:
V1
-
0
|
-
-
EPIX10KA
:
V1
-
1
|
-
-
EPIX10KA
:
V1
-
2
|
-
-
EPIX10KA
:
V1
-
3
|
-
-
QUAD
-
3
|
-
-
EPIX10KA
:
V1
-
0
|
-
-
EPIX10KA
:
V1
-
1
|
-
-
EPIX10KA
:
V1
-
2
|
-
-
EPIX10KA
:
V1
-
3
The quad definitions & hierarchy come from the psana geometry file format.
—
You are receiving this because you were mentioned.
Reply to this email directly, [view it on GitHub](#42 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAEILBIHKOLOBAMU2O3A7BLRYTMQRANCNFSM4N73LKZA).
|
|
I think if we think this is essential, the best way to go is to implement the rigid group reading code, yes. That will be more future-proof and more useful for crystfel too! |
@valmar if you want to take a crack at this, let me know. If not I can do it easily :). This would be a good thing to do together, since afterwards you can probably add any new detector that comes along easily.
All we should need to do is add a new "sensor" to
sensors.py:https://github.com/slaclab/psgeom/blob/master/psgeom/sensors.py
The easy thing to do is follow an example, for instance the JUNGFRAU:
https://github.com/slaclab/psgeom/blob/master/psgeom/sensors.py#L745
Then we need to add tests (this will be the bulk of the work):
https://github.com/slaclab/psgeom/blob/master/test/test_sensors.py
https://github.com/slaclab/psgeom/blob/master/test/test_translate.py
https://github.com/slaclab/psgeom/blob/master/test/test_scripts.py
There is info about the "sensor element" geometry for the Epix10k here:
https://confluence.slac.stanford.edu/display/PSDM/EPIX10KA
There is an example psana geometry file we can use for testing here:
The text was updated successfully, but these errors were encountered: