brisvag / blik

Python tool for visualising and interacting with cryo-ET and subtomogram averaging data.
https://brisvag.github.io/blik/
GNU General Public License v3.0
23 stars 8 forks source link

refactor star file reading #60

Closed alisterburt closed 3 years ago

alisterburt commented 3 years ago

Here's an attempt at refactoring star file IO into something a little more flexible, keen to hear your thoughts

The main ideas behind this implementation are that:

I think that overall this is more explicit than the use of conventions as we were working with before which quickly becomes an unreadable nested if/else mess where interpretation is mixed with parsing. The price is a little bit of code duplication, although some refactoring could be done

relion 3.1 style shifts are now handled inline, optics and particles dataframes are merged so that shifts can be calculated properly - the implementation is robust to the possibility of multiple optics groups (which can have different pixel sizes, happens in single particle sometimes)

One thing that's lost in this implementation is the ability to use your regex wizardry for filename matching, I'm sure it can be added back in I just didn't understand it enough to do so

codecov-io commented 3 years ago

Codecov Report

Merging #60 (598ff49) into develop (2527ea0) will decrease coverage by 0.88%. The diff coverage is 19.67%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #60      +/-   ##
===========================================
- Coverage    54.74%   53.86%   -0.89%     
===========================================
  Files           58       61       +3     
  Lines         1233     1268      +35     
===========================================
+ Hits           675      683       +8     
- Misses         558      585      +27     
Impacted Files Coverage Δ
peepingtom/core/datablocks/orientationblock.py 67.27% <ø> (ø)
peepingtom/io_/read/star/utils.py 15.78% <15.78%> (ø)
peepingtom/io_/read/star/star.py 25.00% <18.18%> (+6.08%) :arrow_up:
peepingtom/io_/read/star/reader_functions.py 33.33% <33.33%> (ø)
peepingtom/__init__.py 100.00% <0.00%> (ø)
peepingtom/entry_points/peep.py 0.00% <0.00%> (ø)
peepingtom/_version.py 100.00% <0.00%> (+100.00%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2527ea0...598ff49. Read the comment docs.

alisterburt commented 3 years ago

this will close #59

should also note that tests should be added - I'll try to add the test data repo with the following tomorrow

alisterburt commented 3 years ago

@brisvag just summarising our discussion a bit here:

my feeling is still that I like these explicit handlings of specific cases despite the presence of common logic, it gives the function single responsibility and is nice and explicit

I think this is now ready to merge!