Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: chxtools package update for 2.0.x #16

Merged
merged 8 commits into from
Dec 20, 2017
Merged

Conversation

mrakitin
Copy link
Member

Here is the updated set of files for chxtools, which now contains the updated list of dependencies which are installed during pip install. There are some outdated files, which I didn't touch, e.g.:

We should probably do something regarding handlers.py and handlers2.py. Either merge them, or use the ones from eiger-io. Ideas?

@mrakitin
Copy link
Member Author

mrakitin commented Dec 20, 2017

@yugangzhang, do you know how to fix this issue? That should be imported from somewhere.

In [4]: import chxtools.chx_utilities
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-4-16a3c9880e42> in <module>()
----> 1 import chxtools.chx_utilities

/home/xf11id/Repos/chxtools/chxtools/chx_utilities.py in <module>()
   1472     gs.TABLE_COLS = [gs.PLOT_Y]
   1473 
-> 1474 def get_ID_calibration(gapstart,gapstop,xray_eye1=xray_eye1, gapstep=.2,gapoff=0,sl=300):
   1475     """
   1476     by LW 04/20/2015

NameError: name 'xray_eye1' is not defined

You can change to
def get_ID_calibration(gapstart,gapstop,xray_eye1=None, gapstep=.2,gapoff=0,sl=300):

Copy link
Contributor

@jrmlhermitte jrmlhermitte left a comment

Choose a reason for hiding this comment

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

Looks good. I see lots of style changes too. Was the whole document checked for flake8?

We could consider adding a flake8 checker for Travis (no tests yet).

This PR only shows a small amount of what @mrakitin had to do to get this all working. Good job looks great! :-)

@@ -1,7 +1,8 @@
import numpy as np
import h5py
from pims import FramesSequence, Frame
from databroker import db
from databroker import Broker
from .handlers import LazyEigerHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

eventually, should be
from eiger_io.fs_handler import EigerHandler
for PIMS version
or
from eiger_io.fs_handler_dask import EigerDaskHandler
for dask version.

But let's keep it as is for now. There are some things I need to check back with Yugang on.

Just to summarize what needs to be fixed with this:

  1. The handlers that treat the AD_EIGER and AD_EIGER2 specs should be combined into one. The only difference between the two is that the keyword argument is a different name. (Other that that API is same as far as I can tell and I've already tested this on all versions of EIGER files, of which there are four in CHX currently)
  2. The EigerImages pims subclass (which is created by the handler per event) contains a lot of metadata which is obtained by reading from the hdf5 file directly. We should discourage that and try to make sure CHX accesses this data directly from metadatastore. Most of this data should be in a descriptor configuration. I want to discuss with Yugang and try my best to make sure we weed this out, and make a long term plan for deprecating this use.

Copy link
Contributor

Choose a reason for hiding this comment

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

db.reg.deregister_handler('AD_EIGER')
db.reg.deregister_handler('AD_EIGER2')

db.reg.register_handler('AD_EIGER2', EigerHandler2)
Copy link
Contributor

Choose a reason for hiding this comment

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

you could remove the previous two lines and add the overwrite=True flag to this line and bellow. Maybe we should get @danielballan 's blessing first ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Blessed!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@@ -10,8 +10,8 @@
import numpy as np
import time

from skxray.core import roi
from skxray.core.utils import bin_edges_to_centers, geometric_series
from skbeam.core import roi
Copy link
Contributor

Choose a reason for hiding this comment

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

for some history: scikit-xray was renamed to scikit-beam a while ago. So just substituting the name is perfect.

@@ -7,6 +7,13 @@

from setuptools import setup, find_packages

no_git_reqs = []
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jrmlhermitte
Copy link
Contributor

For @mrakitin 's previous comment. Just noting that we understood that xray_eye1 is an ophyd object assumed to be in the name space. This should be changed.

@jrmlhermitte
Copy link
Contributor

one more comment : fix the readme if you can (there are three titles)

db.reg.deregister_handler('AD_EIGER')
db.reg.deregister_handler('AD_EIGER2')

db.reg.register_handler('AD_EIGER2', EigerHandler2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Blessed!

@@ -16,19 +17,19 @@ def det_select(det):
calling sequence: det_select(det)
"""
try:
rm_det=ascan.user_detectors[0].name
ascan.user_detectors.remove(session_mgr[rm_det])
rm_det=ascan.user_detectors[0].name
Copy link
Contributor

Choose a reason for hiding this comment

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

This is using very old ophyd API that definitely no longer works. It took me a minute to even remember .user_detectors; I think it goes back to the days when the RunEngine was part of ophyd and there was no such thing as plans. @mrakitin Can you open an issue to revisit this function?

@@ -43,30 +44,32 @@ def cw_ascan(mot,xmin,xmax,npoints,acqt='default',pos_ret=True):
WILL NOT WORK FOR A LIST OF DETECTORS!
"""
# gather beamline information prior to starting the scan:
ini_motpos=caget(mot.record+'.RBV')
ini_motpos = caget(mot.record+'.RBV')
Copy link
Contributor

Choose a reason for hiding this comment

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

caget shouldn't be used in a plan. @mrakitin Can you open an issue for this too?

# put beamline back into initial state
if pos_ret==True:
if pos_ret is True:
Copy link
Contributor

Choose a reason for hiding this comment

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

if pos_ret: is sufficient here.

@@ -78,25 +81,27 @@ def cw_dscan(mot,mdx,pdx,npoints,acqt='default',pos_ret=True):
WILL NOT WORK FOR A LIST OF DETECTORS!
"""
# current detector:
acq_pv=session_mgr[ascan.user_detectors[0].name].pvname

acq_pv = session_mgr[ascan.user_detectors[0].name].pvname
Copy link
Contributor

Choose a reason for hiding this comment

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

session_mgr is long-gone too. I guess we should just make a note to revisit this entire file. I suspect it hasn't been used in years, so we need to decide whether to delete it or overhaul it.

@yugangzhang
Copy link
Collaborator

Great Job!

@yugangzhang yugangzhang merged commit 0261082 into master Dec 20, 2017
@danielballan
Copy link
Contributor

@yugangzhang There were some comments that we had planned to address before merging here.

@yugangzhang
Copy link
Collaborator

@danielballan I think there might be some issues with handler, but it's in eiger_io package. I will revert it.

@mrakitin mrakitin deleted the enh_chxtools_pkg branch December 20, 2017 22:25
This was referenced Dec 20, 2017
@mrakitin
Copy link
Member Author

I implemented suggestions from @jrmlhermitte and @danielballan, see #18.

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.

4 participants