Skip to content

Conversation

@vineetbansal
Copy link
Contributor

@vineetbansal vineetbansal commented Apr 8, 2023

While moving the relion code from pandas to straight dicts, I noticed what looks to me like a bug in the way the optics properties are expanded by optics group lookup (unless I'm misunderstanding the spec). The current implementation is assuming ordered rows in the data_optics table that go from 1-n as far as _rlnOpticsGroup is concerned. Swapping (or shuffling in the general case) the rows should have no effect, but does, as is demonstrated by the failing test here.

I'm breaking this into two PRs so they're easier to reason through - one for the expected failing test, the other for the fix that takes the xfail off.

@codecov
Copy link

codecov bot commented Apr 8, 2023

Codecov Report

Merging #909 (27a4de9) into develop (e3145c1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #909   +/-   ##
========================================
  Coverage    88.18%   88.18%           
========================================
  Files          119      119           
  Lines        10118    10118           
========================================
  Hits          8923     8923           
  Misses        1195     1195           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@garrettwrong
Copy link
Collaborator

garrettwrong commented Apr 10, 2023

Thanks Vineet. Can you point me to to spec you are referencing? Here I don't see anything specific (which makes me think your proposed fix is safer), but I'm curious if Relion actually allows this situation.

(forgot to paste link)

https://relion.readthedocs.io/en/release-3.1/Reference/Conventions.html#optics-groups

@vineetbansal
Copy link
Contributor Author

Oh - that was the document I was referring to (the line "..refer to an optics group entry in the optics table via the rlnOpticsGroup column"). I don't know of the official spec beyond that one.

@garrettwrong
Copy link
Collaborator

Closing, we'll take #910, #911 or both, which contain this.

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.

3 participants