-
Notifications
You must be signed in to change notification settings - Fork 3
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
0.4.0 Previewer #92
base: dev
Are you sure you want to change the base?
0.4.0 Previewer #92
Conversation
…UI from old magicgui.
…55/.local/bin:/home/wed13955/bin:/usr/local/go/bin:/root/go/bin:/usr/local/cuda/bin:/opt/lmod/modules/miniconda/4.10.3/bin:/opt/lmod/modules/miniconda/4.10.3/condabin:/usr/local/go/bin:/root/go/bin:/usr/local/cuda/bin:/usr/local/go/bin:/root/go/bin:/usr/local/cuda/bin:/sbin:/bin:/usr/bin:/usr/local/bin:/snap/bin:/ceph/groups/structbio/ot2rec-0.2.4/bin.
…e/wed13955/.local/bin:/home/wed13955/bin:/usr/local/go/bin:/root/go/bin:/usr/local/cuda/bin:/opt/lmod/modules/miniconda/4.10.3/bin:/opt/lmod/modules/miniconda/4.10.3/condabin:/usr/local/go/bin:/root/go/bin:/usr/local/cuda/bin:/usr/local/go/bin:/root/go/bin:/usr/local/cuda/bin:/sbin:/bin:/usr/bin:/usr/local/bin:/snap/bin:/ceph/groups/structbio/ot2rec-0.2.4/bin." This reverts commit 88a1a0b.
…UI from old magicgui.
…UI from old magicgui.
…d tomogram thickness to binned thickness.
…MC2->AreTomo route.
…cluded in commit f48a1e1.
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.
Looks good overall, thanks Neville - a few bits and pieces to look at which are in the comments.
Main thing to consider is if we should separate out the previewer code into its own previewer.py
because that would make it more consistent with how the other plugins are set up. it'll be a "meta plugin" but still works the same as the others and would be easier to maintain if it's separated out like the others.
I'll update the tests and add ot2rec_report to the previewer in a separate PR into this branch to keep this PR a manageable size
@@ -166,10 +167,17 @@ def create_stack_folders(self): | |||
|
|||
# Create the folders and dictionary for future reference | |||
self._path_dict = {} | |||
for curr_ts in self._process_list: | |||
subfolder_path = f'{self.basis_folder}/{self.rootname}_{curr_ts:04}{self.suffix}' | |||
if single_folder: |
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.
Does this mean that if you are processing images which are in a single folder (without the tilt series named subfolders), the processed data will live in a single folder as well rather than be organised into individual tilt series? We would have to account for this in the downstream plugins as well (maybe you have and I haven't gotten to that part yet...)
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, but only if so wished by the user. I've never had problems with this one as by default they are still organised into subfolders.
src/Ot2Rec/align.py
Outdated
'use_rawtlt': 1 if self.params['BatchRunTomo']['setup']['use_rawtlt'] else 0, | ||
'pixel_size': self.params['BatchRunTomo']['setup']['pixel_size'], | ||
'pixel_size': self.params['BatchRunTomo']['setup']['pixel_size'] * self.params['BatchRunTomo']['setup']['stack_bin_factor'], |
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.
Does this accidentally apply binning twice, since binning is already passed to batchruntomo
with <aligned_bin_factor>
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.
No. What it corrects is the wrong pixels size being passed into IMOD. This is because I've modified the code such that you can pre-bin the unaligned stacks first then do alignment. In that case, the old module would have the unbinned pixel size for alignment which is wrong.
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 aligned_bin_factor is for the post-alignment stack (i.e. _.ali.mrc), whereas the stack_bin_factor is for the pre-aligned one (i.e. .st).
src/Ot2Rec/aretomo.py
Outdated
@@ -277,7 +283,7 @@ def _update_volz(args, aretomo_params): | |||
in nm to automatically calculate VolZ. Currently sample_thickness \ | |||
= {args['sample_thickness']} and pixel_size = {args['pixel_size']}") | |||
aretomo_params.params["AreTomo_recon"]["volz"] = int( | |||
(args["sample_thickness"] * args["pixel_size"] * 0.1) + 200 # factor of 0.1 because pixel_size in A | |||
(10 * args["sample_thickness"] // args["pixel_size"]) + 200 # factor of 0.1 because pixel_size in A |
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.
Does args["pixel_size"]
account for bin factors here?
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.
No, because by definition VolZ is unbinned thickness in pixels
src/Ot2Rec/aretomo.py
Outdated
@@ -30,6 +30,12 @@ | |||
from . import magicgui as mgMod | |||
from . import params as prmMod | |||
from . import user_args as uaMod | |||
from . import mgui_imod_align as alignMGUI |
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 think it might be clearer to import this as align_args
or something similar
src/Ot2Rec/main.py
Outdated
meta.get_mc2_temp() | ||
meta.get_acquisition_settings() | ||
|
||
master_md_name = new_proj_params.project_name + '_master_md.yaml' |
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.
not for now but in the future maybe we should be consistent with one of either f strings or the + notation like here (my preference is fstrings but can decide later)
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.
Yeah this is old stuff, I'm using f-strings recently
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 think we could leave these minor stylistic polishing till later when the main codebase is more stable. There will be many of them in the modules as well, not just the main API.
src/Ot2Rec/aretomo.py
Outdated
@@ -290,7 +296,7 @@ def _update_volz(args, aretomo_params): | |||
def _create_stacks_with_imod(args): | |||
# Uses align to create the InMrc and AngFile in correct form | |||
try: | |||
args_in_align = mgMod.get_args_align | |||
args_in_align = alignMGUI.get_args_align |
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.
would this bring up the magicgui? in main.py
you've used return_only=True
into get_args
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.
That would. However I'm not using this subroutine, but my old one directly from IMOD newstack (in align.py) instead so the GUI wouldn't come up like when you do command-line o2r.aretomo.run project 2
.
src/Ot2Rec/main.py
Outdated
mcMod.run(exclusive=False, | ||
args_in=mc2_params) | ||
|
||
time.sleep(2) |
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.
what's this for?
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.
Leave 2 seconds between processes; I don't like automated to processes to run straight after one another.
src/Ot2Rec/mgui_mc2.py
Outdated
max_iter=10, | ||
patch_size=[5,5,20], | ||
use_subgroups=True, | ||
*, |
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.
what's the * for?
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 a separator
…tack_MGUI in aretomo.
…ct parsing in metadata.
Change log
Note