-
Notifications
You must be signed in to change notification settings - Fork 12
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
Bugfix hkl #11
Bugfix hkl #11
Conversation
tardis.calc['delta']=-10,100, set value or limits? |
it sets the limits |
hkl/calc.py
Outdated
p = self._geometry.axis_get(name) | ||
# the third param, "1" means units are degrees | ||
# "0" means radians. | ||
p.min_max_set(value[0], value[1], 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we use unit as 0 or 1?
Could you also fix this by passing the geometry object + an axis name into the |
8b2943e
to
d48a3ab
Compare
Thanks @tacaswell the previous implementation was incorrect. |
hkl/engine.py
Outdated
if self._inverted: | ||
low, high = -high, -low | ||
self._param.min_max_set(low, high, self._units) | ||
if self._geometry is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a raise error on geometry in init, so self._geometry will not be None?
hkl/engine.py
Outdated
if self._inverted: | ||
value *= -1.0 | ||
|
||
if self._geometry is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here?
hkl/engine.py
Outdated
def __init__(self, param, *args, geometry=None, **kwargs): | ||
''' | ||
Like calc parameter but needs reference to a geometry object. | ||
Updates to the parameter should be propagated back to the geometry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add some docs on param and geometry?
hkl/engine.py
Outdated
self.param_name = param.name_get() | ||
# you may wonder why we stash geometry object rather than axis | ||
# this is because the axis params can only be updated from the geometry | ||
if geometry is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or make geometry not as optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. i made it required
@property | ||
def fit(self): | ||
'''True if the parameter can be fit or not''' | ||
axis = self._geometry.axis_get(self.param_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it is easier to move this line to init? axis is used in every function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, we can't. axis_get
returns a copy every time. We need to fetch an updated copy at the time of modification. It would also probably be good to use a locking mechanism to ensure no one else is modifying the object as this is happening in the long run. (like epics callbacks which are not GIL locked). but we should see what CSX needs first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all python with in a given process uses the same GIL. If it were not there would be segfaults all over the place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the threads that ran the epics callbacks were not GIL locked because they ran in some C thread. so they are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, part of interpreting python byte code (from any thread) is acquiring the GIL.
some comments. thanks for the work! |
thanks, sounds good, i added a test as well (not tested with travis just tested manually) |
LGTM. We also tested it at CSX, and everything works. Let's also wait for other's input? |
yes, i would say this is good to merge. We can wait for @tacaswell or others again. But let's aim to get this before next coming cycle. |
Can you squash this down to one or two commits with good commit messages and put the tests is a function? |
Ensure that changes to value, limits and fit update the parent axis added tests, force geometry to be given to CalcParameter
f3a0c58
to
b69caad
Compare
ok. I just noticed there were tests one level up. In |
.travis.yml
Outdated
@@ -44,6 +44,8 @@ install: | |||
# ophyd version not tagged publicly just yet: | |||
# (dependencies pulled in above) | |||
- pip install git+http://github.com/nsls-ii/ophyd/ | |||
# this is an ophyd dep | |||
- pip install typing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the standard library as of 3.5 which is the minimum that ophyd / bluesky currently support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple small suggestions. Looks good otherwise.
hkl/calc.py
Outdated
@@ -405,10 +405,11 @@ def units(self): | |||
return self._unit_name | |||
|
|||
def __getitem__(self, axis): | |||
return Parameter(self._get_axis_by_name(axis), | |||
return CalcParameter(self._get_axis_by_name(axis), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whitespace is funny here.
hkl/engine.py
Outdated
axis = self._geometry.axis_get(self.param_name) | ||
low, high = axis.min_max_get(self._units) | ||
if self._inverted: | ||
return [-high, -low] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path returns a list
and the other path returns tuple
. Aside from the general benefits of consistency, return mutable types (like list
) have been the source of confounding bugs in the past. Better make this one a tuple: -high, -low
.
.travis.yml
Outdated
|
||
- pip install coveralls pytest pytest-cov | ||
- python setup.py develop | ||
|
||
# this is important: | ||
- conda install -c lightsource2 readline | ||
#- conda install -c lightsource2-tag readline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that this is now commented out seems out of sync with the remark above it ("this is important"). Should it be removed entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, i changed it to lightsource2-tag
out when testing, then found i didn't need it so commented it out. @tacaswell can you confirm we don't need this anymore? worked for me when testing locally, and works on Travis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer important because we have removed the readline linking in epics-base so we should no longer have issues with crossed versions of readline. There may still need to be one more local change applied to our pyepics, but we should be able to use the stock readline again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great thanks! in either case that was for Travis, and it passed without. great job in solving that annoying problem!
Thanks @danielballan . I made the changes. I'll wait for @tacaswell 's comment on readline ( here ) |
hkl/engine.py
Outdated
@@ -267,3 +267,70 @@ def _repr_info(self): | |||
def __repr__(self): | |||
return '{}({})'.format(self.__class__.__name__, | |||
', '.join(self._repr_info())) | |||
|
|||
# the parameter for the calc object | |||
# this is a fix for backwards compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a back-compatibilty fix
.travis.yml
Outdated
@@ -9,8 +9,9 @@ cache: | |||
|
|||
matrix: | |||
include: | |||
- python: 3.4 | |||
- python: 3.5 | |||
# we have removed the py35 dep because it does not work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just remove these
.travis.yml
Outdated
|
||
# ophyd version not tagged publicly just yet: | ||
# (dependencies pulled in above) | ||
- pip install git+http://github.com/nsls-ii/ophyd/ | ||
#- pip install ophyd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, just remove this line
.travis.yml
Outdated
- conda config --add channels soft-matter | ||
|
||
# MAKE THE CONDA RECIPE | ||
- conda create -n $CONDA_ENV python=$TRAVIS_PYTHON_VERSION | ||
- source activate $CONDA_ENV | ||
|
||
install: | ||
- conda install numpy pyepics prettytable filestore ipython pyolog pip | ||
- conda install numpy pyepics prettytable ipython pyolog pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need pyolog! Not worth addressing...
This fixes a bug in hklpy. Fix the setting of the limits by using the API of hklpy.
Previously, we were doing something like:
This is incorrect, as the
Geometry
object does not return a pointer to the axis, but rather a copy. After the modification, one should re-insert it with:This somehow worked with previous versions of hklpy. We suspect that this has something to do with the gobject library wrapping the C modules into python, but we're not sure.
I verified with the simuated tardis here: