Helpful error detection from sub-processes

src/matchmaps/_compute_mr_diff.py

@@ -1,6 +1,5 @@
 """Compute unbiased real space difference map from inputs in different spacegroups"""
 
-import argparse
 import os
 import sys
 import time
@@ -126,7 +125,7 @@ def compute_mr_difference_map(
 
     # write this function as a wrapper around phenix.pdbtools
     # modified pdboff already moved to output_dir by _handle_special_positions
-    pdboff = _remove_waters(pdboff, output_dir)
+    pdboff = _remove_waters(pdboff, output_dir, verbose)
 
     print(
         f"{time.strftime('%H:%M:%S')}: Running phenix.phaser to place 'off' model into 'on' data..."

src/matchmaps/_compute_ncs_diff.py

@@ -108,7 +108,7 @@ def compute_ncs_difference_map(
 
         pdb = _handle_special_positions(pdb, output_dir)
 
-        pdb = _renumber_waters(pdb)
+        pdb = _renumber_waters(pdb, verbose)
 
         print(f"{time.strftime('%H:%M:%S')}: Running phenix.refine...")
 

src/matchmaps/_compute_realspace_diff.py

@@ -1,7 +1,5 @@
 """Compute unbiased real space difference map."""
 
-import argparse
-import subprocess
 import sys
 import time
 from pathlib import Path
@@ -10,7 +8,7 @@
 import reciprocalspaceship as rs
 
 from matchmaps._parsers import matchmaps_parser
-from matchmaps._phenix_utils import rigid_body_refinement_wrapper, _renumber_waters
+from matchmaps._phenix_utils import rigid_body_refinement_wrapper, _renumber_waters, _custom_subprocess
 from matchmaps._utils import (
     _handle_special_positions,
     make_floatgrid_from_mtz,
@@ -127,20 +125,20 @@ def compute_realspace_difference_map(
         f"{time.strftime('%H:%M:%S')}: Running scaleit to scale 'on' data to 'off' data..."
     )
 
-    subprocess.run(
-        f"rs.scaleit -r {mtzoff} {Foff} {SigFoff} -i {mtzon} {Fon} {SigFon} -o {mtzon_scaled} --ignore-isomorphism",
-        shell=True,
-        capture_output=(not verbose),
+    _custom_subprocess(
+        command="rs.scaleit",
+        params=f"-r {mtzoff} {Foff} {SigFoff} -i {mtzon} {Fon} {SigFon} -o {mtzon_scaled} --ignore-isomorphism",
+        verbose=verbose
     )
 
-    ## now that scaleit has run, let's swap out the spacegroup from the scaled file
+    # now that scaleit has run, let's swap out the spacegroup from the scaled file
     mtzon_scaled_py = rs.read_mtz(str(mtzon_scaled))
     mtzon_original_py = rs.read_mtz(str(mtzon))
     mtzoff_original_py = rs.read_mtz(str(mtzoff))
-    
+
     mtzoff_trunc = output_dir / (mtzoff.name.removesuffix(".mtz") + "_trunc.mtz")
     mtzon_scaled_truecell = output_dir / (mtzon_scaled.name.removesuffix(".mtz") + "_truecell.mtz")
-    
+
     mtzon_scaled_py.cell = mtzon_original_py.cell
 
     mtzoff_original_py.compute_dHKL(inplace=True)
@@ -157,11 +155,11 @@ def compute_realspace_difference_map(
     # reset short nicknames to the latest files
     mtzon = mtzon_scaled_truecell
     mtzoff = mtzoff_trunc
-    ## done with cell swapping and resolution matching
+    # done with cell swapping and resolution matching
 
     pdboff = _handle_special_positions(pdboff, output_dir)
 
-    pdboff = _renumber_waters(pdboff)
+    pdboff = _renumber_waters(pdboff, verbose)
 
     print(f"{time.strftime('%H:%M:%S')}: Running phenix.refine for the 'on' data...")
 
@@ -213,12 +211,13 @@ def compute_realspace_difference_map(
     # TO-DO: Figure out why phenix outputs are sometimes still split into (+) and (-) columns, even when I specify that anomalous=False
     # As a workaround, even anomalous files have a single 'F-obs-filtered' column, so I can always just use that.
     fg_off = make_floatgrid_from_mtz(
-        mtzoff, spacing, F="F-obs-filtered", SigF="SIGF-obs-filtered", Phi="PH2FOFCWT", spacegroup="P1", dmin=dmin, alpha=alpha,
+        mtzoff, spacing, F="F-obs-filtered", SigF="SIGF-obs-filtered", Phi="PH2FOFCWT", spacegroup="P1", dmin=dmin,
+        alpha=alpha,
     )
     fg_on = make_floatgrid_from_mtz(
-        mtzon, spacing, F="F-obs-filtered", SigF="SIGF-obs-filtered", Phi="PH2FOFCWT", spacegroup="P1", dmin=dmin, alpha=alpha,
+        mtzon, spacing, F="F-obs-filtered", SigF="SIGF-obs-filtered", Phi="PH2FOFCWT", spacegroup="P1", dmin=dmin,
+        alpha=alpha,
     )
-
 
     if rbr_gemmi is None:
         _realspace_align_and_subtract(
@@ -251,7 +250,7 @@ def compute_realspace_difference_map(
                 selection=selection,
                 radius=radius,
             )
-    
+
     print(f"{time.strftime('%H:%M:%S')}: Cleaning up files...")
     _clean_up_files(output_dir, output_dir_contents, keep_temp_files)
 
@@ -271,7 +270,7 @@ def main():
         args.mtzon[0],
         args.pdboff,
     )
-    
+
     compute_realspace_difference_map(
         pdboff=pdboff,
         ligands=ligands,
@@ -292,17 +291,17 @@ def main():
         alpha=args.alpha,
         on_as_stationary=args.on_as_stationary,
         keep_temp_files=args.keep_temp_files,
-        no_bss = args.no_bss,
-        phenix_version = args.phenix_version,
+        no_bss=args.no_bss,
+        phenix_version=args.phenix_version,
     )
-    
+
     if args.script:
         _write_script(
-            utility = 'matchmaps', 
-            arguments = sys.argv[1:],
-            script_name = args.script,
+            utility='matchmaps',
+            arguments=sys.argv[1:],
+            script_name=args.script,
             phenix_version=args.phenix_version,
-            )
+        )
 
     return
 

src/matchmaps/_phenix_utils.py

@@ -7,7 +7,7 @@
 
 
 def _auto_eff_refinement_template(phenix_style: str):
-    if phenix_style == '1.20':
+    if phenix_style == "1.20":
         eff_contents = """
     refinement {
       crystal_symmetry {
@@ -65,7 +65,7 @@ def _auto_eff_refinement_template(phenix_style: str):
       }
     }
         """
-    elif phenix_style == '1.21':
+    elif phenix_style == "1.21":
         eff_contents = """
 data_manager {
   model {
@@ -132,24 +132,24 @@ def _auto_eff_refinement_template(phenix_style: str):
 }
     """
     else:
-        raise NotImplementedError('Unsupported phenix version')
+        raise NotImplementedError("Unsupported phenix version")
 
     return eff_contents
 
 
 def rigid_body_refinement_wrapper(
-        mtzon,
-        pdboff,
-        input_dir,
-        output_dir,
-        phenix_style,
-        off_labels=None,
-        ligands=None,
-        eff=None,
-        verbose=False,
-        rbr_selections=None,
-        mr_on=False,
-        no_bss=False,
+    mtzon,
+    pdboff,
+    input_dir,
+    output_dir,
+    phenix_style,
+    off_labels=None,
+    ligands=None,
+    eff=None,
+    verbose=False,
+    rbr_selections=None,
+    mr_on=False,
+    no_bss=False,
 ):
     if eff is None:
         eff_contents = _auto_eff_refinement_template(phenix_style=phenix_style)
@@ -215,17 +215,18 @@ def rigid_body_refinement_wrapper(
         file.write(eff_contents)
     # run refinement!
     # print refinement output to terminal if user supplied the --verbose flag
-    subprocess.run(
-        f"phenix.refine {eff}",
-        shell=True,
-        capture_output=(not verbose),
+
+    _custom_subprocess(
+        command="phenix.refine",
+        params=str(eff),
+        verbose=verbose,
     )
 
     return output_dir / nickname
 
 
 def _auto_eff_phaser_template(phenix_style):
-    if (phenix_style == '1.20') or (phenix_style == '1.21'):
+    if (phenix_style == "1.20") or (phenix_style == "1.21"):
         eff_contents = """
 phaser {
   mode = ANO CCA EP_AUTO *MR_AUTO MR_FRF MR_FTF MR_PAK MR_RNP NMAXYZ SCEDS
@@ -253,22 +254,23 @@ def _auto_eff_phaser_template(phenix_style):
   }
 }
         """
-    elif phenix_style == '1.21':
+    elif phenix_style == "1.21":
         eff_contents = """"""
 
     else:
         raise NotImplementedError(f"Phenix version {phenix_style} not supported")
 
     return eff_contents
 
+
 def phaser_wrapper(
-        mtzfile,
-        pdb,
-        output_dir,
-        off_labels,
-        phenix_style,
-        eff=None,
-        verbose=False,
+    mtzfile,
+    pdb,
+    output_dir,
+    off_labels,
+    phenix_style,
+    eff=None,
+    verbose=False,
 ):
     """
     Handle simple phaser run from the command line
@@ -322,10 +324,10 @@ def phaser_wrapper(
     with open(eff, "w") as file:
         file.write(eff_contents)
 
-    subprocess.run(
-        f"phenix.phaser {eff}",
-        shell=True,
-        capture_output=(not verbose),
+    _custom_subprocess(
+        command="phenix.phaser",
+        params=str(eff),
+        verbose=verbose
     )
 
     return output_dir / nickname
@@ -338,7 +340,7 @@ def _parse_mtz(mtzfile):
     return cell_string, sg
 
 
-def _renumber_waters(pdb):
+def _renumber_waters(pdb, verbose):
     """
     Call phenix.sort_hetatms to place waters onto the nearest protein chain.
     This ensures that rbr selections handle waters properly
@@ -349,14 +351,15 @@ def _renumber_waters(pdb):
         name of pdb file
     dir : str
         directory in which pdb file lives
+    verbose:
     """
 
     pdb_renumbered = Path(str(pdb).removesuffix(".pdb") + "_renumbered.pdb")
 
-    subprocess.run(
-        f"phenix.sort_hetatms file_name={pdb} output_file={pdb_renumbered}",
-        shell=True,
-        capture_output=True,
+    _custom_subprocess(
+        command='phenix.sort_hetatms',
+        params=f"file_name={pdb} output_file={pdb_renumbered}",
+        verbose=verbose
     )
 
     print(f"{time.strftime('%H:%M:%S')}: Moved waters to nearest protein chains...")
@@ -365,17 +368,37 @@ def _renumber_waters(pdb):
 
 
 def _remove_waters(
-        pdb,
-        output_dir,
+    pdb,
+    output_dir,
+    verbose
 ):
     pdb_dry = pdb.name.removesuffix(".pdb") + "_dry"
 
-    subprocess.run(
-        f"phenix.pdbtools {pdb} remove='water' \
+    _custom_subprocess(
+        command="phenix.pdbtools",
+        params=f"{pdb} remove='water' \
             output.prefix='{output_dir}/' \
             output.suffix='{pdb_dry}'",
-        shell=True,
-        capture_output=True,
+        verbose=verbose
     )
 
     return output_dir / (pdb_dry + ".pdb")
+
+
+def _custom_subprocess(command, params, verbose, shell=True):
+
+    subproc = subprocess.run(
+        command + " " + params, shell=shell, capture_output=(not verbose)
+    )
+
+    if subproc.returncode != 0:
+
+        error = f"matchmaps encountered an error while running {command}" + (
+            "\n              Try again in --verbose mode for more a more detailed error message"
+            if (not verbose)
+            else ""
+        )
+
+        raise RuntimeError(error)
+
+    return