Distance

[chuckie82]

1) to_crystfel_file() now requires coffset (m) This is because psana geometry only has detector distance, but crystfel requires clen and coffset. clen is usually a string to hdf5 dataset. coffset is explicitly required.

2) to_psana_file() now requires detector distance (m) This is because crystfel geom has clen and coffset. Since clen is usually a string to hdf5 dataset, you can not infer the value of detector distance from coffset.

[tjlane]

I think we should do this -- here's my thoughts:

• we should make coffset a default, we need to maintain common interface between different formats as much as possible
• since people may edit the resulting crystfel geom file to add clen, can we expect them to modify coffset?
• needs documentation before merge

[chuckie82]

• I agree we should maintain a common interface. We can set coffset to zero, but then it's up to the user to correct it.

FYI, CompoundCamera and Cspad classes are the only two that call to_crystfel_file(). I suspect any future class that calls this function will require a coffset as an input. So the current PR maintains a common interface. Thoughts?

• Yes, they may want to modify coffset manually. Currently, coffset is set per asic which is hard for users to manually modify. I suggest we have one coffset at the top.
• Mo' documentation :)

[chuckie82]

Set defaults to 0.0 Added documentation

[tjlane]

Three remaining comments:

• there is a line with a hard-coded "QUAD:V1" that seems to also assume a structure in the parent. Can we modify this line by changing the distance of the child right below "IP"? In most cases that will be "CSPAD", but we don't need to assume that.
• should we add a small test? have you done an offline test to make sure it all works ok?
• big one: is it possible to change the base branch of this PR from master to newdets? I think we should pull it into newdets, then finish that work and get it in master.

[chuckie82]

• I discussed with @msdubrovin about point 1.We checked 3 cspad geometry files for cxi, xpp and mec. It seems IP is the common place we can set the detector distance. If no objections, I'll use this line to set Z.
• It's been tested offline.
• If I'm not mistaken, I think this is setup to merge into newdets. chuckie82 wants to merge 5 commits into slaclab:newdets from chuckie82:distance

[chuckie82]

I think test.py is broken (in this branch?). TJ, could you add these tests?

from psgeom import camera cspad = camera.Cspad.from_crystfel_file("ref_files/refgeom_crystfel.geom") cspad.to_psana_file('ref_files/temp.data') # Set distance to default 1 metre

from psgeom import camera cspad = camera.Cspad.from_crystfel_file("ref_files/refgeom_crystfel.geom") cspad.to_psana_file('ref_files/temp.data', dist=0.75) # Set distance to 0.75 metres

from psgeom import camera geom = camera.Cspad.from_psana_file('ref_files/refgeom_psana.data') geom.to_crystfel_file('temp.geom') # Set coffset to default 0.0

from psgeom import camera geom = camera.Cspad.from_psana_file('ref_files/refgeom_psana.data') geom.to_crystfel_file('temp.geom',coffset=0.585) # Set coffset to 0.585 metres

[tjlane]

@chuckie82 seems i do not have permissions to update your fork. I added the test but can't push :(. It is in the local repo in a new branch, chuckie82-distance, which you should be able to merge.

To run the tests: nosetest.py test.py

It seems that these changes broke 4 tests. The "default" distance seems to be overriding z-distances that were previously correctly inferred from the geometry files. Run the tests and see if you agree.

[chuckie82]

Do you have your own nose installed? I couldn't run it on psana interactive nodes.

[yoon82@psanaphi101 psgeom]$ nosetests test.py Traceback (most recent call last): File "/usr/bin/nosetests", line 5, in from pkg_resources import load_entry_point File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 2867, in File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 432, in _build_master File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 728, in require File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 622, in resolve pkg_resources.DistributionNotFound: nose==1.3.0

[tjlane]

Yeah i do have my own version... seems like we should get the psana version working though!

Easy to install if you have a virtualenv set up.

[chrisvam]

On Oct 31, 2016, at 6:59 AM, TJ Lane notifications@github.com<mailto:notifications@github.com> wrote:

Yeah i do have my own version... seems like we should get the psana version working though!

Easy to install if you have a virtualenv set up.

We could also see if it works in David’s anaconda version (they way of the future, perhaps):

https://confluence.slac.stanford.edu/display/PSDMInternal/Conda+based+Psana+in+LCLS+central+install

chris

[chuckie82]

I tried testing with nose through anaconda and it seems to work. The tests fail, but at least the first few failures don't seem to have anything to do with my commits.

@tjlane Shall we sit together sometime this week and fix up the tests? Tues or Wed?

[tjlane]

@chuckie82 before we meet up try checking out newdets or master (w/o these changes) and run the tests. When I did this, they worked. They seem to fail due to changes along the z-axis...

Time early this week tough due to beamtime. If Chris is flexible: 2pm Wed?

[chrisvam]

On Oct 31, 2016, at 2:48 PM, TJ Lane notifications@github.com<mailto:notifications@github.com> wrote:

If Chris is flexible: 2pm Wed?

Works me me…

c

[chuckie82]

OK, let's meet up on Wed 2pm. I'll test the Rayonix conversion in the mean time.

This is the output from newdets w/o my changes: HEAD is now at 8662b05... added non-cspad support to geoconv FAILED (SKIP=1, failures=3)

[chuckie82]

Discussion with TJ and Chuck to_crystfel_file() sets coffset to dist (default) to_crystfel_file(coffset) sets coffset to coffset

To go back to psana geometry from crystfel geom: translate(0,0,Z) to_psana_file()

[chuckie82]

Changed default behavior as discussed.

Test still fails due to differences in from_psana_file() and Mikhail's get_pixel_coords(), unrelated to my commits. Safe to merge?