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

Remove cache of Kabsch debugging #1761

Merged
merged 7 commits into from
Jun 28, 2021
Merged

Remove cache of Kabsch debugging #1761

merged 7 commits into from
Jun 28, 2021

Conversation

graeme-winter
Copy link
Contributor

Was broken anyway for stills as image.index is always 0 so did not update when moving to next frame.

Fixes #1734

Removed now redundant request dict as well

Was broken anyway for stills as image.index is always 0

Fixes #1734

Removed now redundant request dict as well
@phyy-nx
Copy link
Member

phyy-nx commented Jun 22, 2021

Wouldn't this re-open #1213? (@dagewa)

@graeme-winter
Copy link
Contributor Author

Wouldn't this re-open #1213? (@dagewa)

Could be, but working and slow seems better to me than broken and quick

We know the image viewer as a whole needs speeding up 🙄

@dagewa
Copy link
Member

dagewa commented Jun 22, 2021

Yes, it would reopen #1213. What does change when moving through stills if not image.index? It seems this could be fixed less destructively if the right item is put in the self._dispersion_debug_memo dictionary.

@dagewa
Copy link
Member

dagewa commented Jun 22, 2021

Is there an example dataset available somewhere I can look at to demonstrate #1734?

@codecov
Copy link

codecov bot commented Jun 22, 2021

Codecov Report

Merging #1761 (50284ad) into main (e9b1e2f) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 50284ad differs from pull request most recent head d731ca1. Consider uploading reports for the commit d731ca1 to get more accurate results

@@           Coverage Diff           @@
##             main    #1761   +/-   ##
=======================================
  Coverage   67.22%   67.23%           
=======================================
  Files         616      616           
  Lines       69021    69012    -9     
  Branches     9618     9618           
=======================================
  Hits        46399    46399           
+ Misses      20680    20671    -9     
  Partials     1942     1942           

@graeme-winter
Copy link
Contributor Author

Yes, it would reopen #1213. What does change when moving through stills if not image.index? It seems this could be fixed less destructively if the right item is put in the self._dispersion_debug_memo dictionary.

Still process imported experiments each have length 1 imageset -> index always 0, you iterate through imagesets. I will come up with a less destructive fix

@graeme-winter
Copy link
Contributor Author

Reproduce: pick any sequence of still images, run through dials.stills_process then

dials.image_viewer *imported.expt

stills processing need not have worked as the data will still have been imported

By simply computing a hash of the settings including the imageset this still
fixes the bug whilst leaving the other thing still fixed. Changing a request
dict to a simple hash of a tuple of the settings stops a future reader
from working through the code looking for where that request is sent to.

Also named the hash the same as the object which is hashed, to make the
connection clearer.

Bug still fixed, no previous bugs restored. I believe these are good changes.
@graeme-winter
Copy link
Contributor Author

@phyy-nx @dagewa I have restored the fix to #1213 but restructured the code rather than aiming for a minimal change. Motivation:

  • before I spent a good chunk of time trying to work out where request was sent - it was sent nowhere
  • computing a hash rather than a dictionary comparison seems more idiomatic for "what was intended"

FWIW simply including the imageset pointer in whatever the fix was would have fixed the initial bug but would mean a future explorer in these woods would have had to retrace steps.

@dagewa
Copy link
Member

dagewa commented Jun 23, 2021

This looks like a better solution. I did wonder if we could use functools.lru_cache for this though, but I'm not sure if it would work with the type passed as image to _calculate_dispersion_debug (which might be something like a "chooser_wrapper")

@graeme-winter
Copy link
Contributor Author

@ndevenish are the build errors related to PR or some external influence? I cannot tell 🤔

@Anthchirp
Copy link
Member

cctbx/cctbx_project#627 to blame

@ndevenish ndevenish enabled auto-merge (squash) June 28, 2021 14:16
@ndevenish ndevenish disabled auto-merge June 28, 2021 15:12
@ndevenish ndevenish merged commit 0db27df into main Jun 28, 2021
@ndevenish ndevenish deleted the kabsch-decache branch June 28, 2021 15:14
ndevenish pushed a commit that referenced this pull request Jun 28, 2021
By simply computing a hash of the settings including the imageset this
still fixes #1734 whilst leaving #1213 fixed. Changing a request dict
to a simple hash of a tuple of the settings stops a future reader from
working through the code looking for where that request is sent to.

Also named the hash the same as the object which is hashed, to make the
connection clearer.
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.

dials.image_viewer does not update results of bottom buttons for series of '.expt'
6 participants