Bugfix and minor cleanup of ResultsSummary code + control SHELXE use depending on resolution #52

linucks · 2018-06-25T12:03:23Z

Needs refactoring but not today...

…value

fsimkovic

Left some comments, think we should also consider either always raise-ing exceptions or calling ample_util.exit_util. Currently it's mixed in the code and I think that's very inconsistent.

My vote is for the former, since that enables others to use AMPLE more like a library with their own error handling rather than applications just exiting.

fsimkovic · 2018-06-26T07:58:33Z

ample/util/options_processor.py

+            optd['tmscore_exe'] = 'TMscore' + ample_util.EXE_EXT
+        try:
+            optd['tmscore_exe'] = ample_util.find_exe(optd['tmscore_exe'])
+            optd['have_tmscore'] = True


Change this to a try - except - else ... we're not really testing the setting of a variable.

try: optd['tmscore_exe'] = ample_util.find_exe(optd['tmscore_exe']) except ample_util.FileNotFoundError: logger.warning("Cannot find TMScore executable: %s", optd['tmscore_exe']) optd['maxcluster_exe'] = maxcluster.find_maxcluster(optd) optd['have_tmscore'] = False else: optd['have_tmscore'] = True

Note the change of logger message from debug to warning, since the former is what we're really looking for.

fsimkovic · 2018-06-26T08:00:24Z

ample/util/options_processor.py

-    # Modelling and ensemble options
-    #
-    ###############################################################################
+    logger.info('Running on %d processors' % optd['nproc'])


Comma-separated instead of %

fsimkovic · 2018-06-26T08:01:28Z

ample/util/options_processor.py

+    # cluster queueing
+    if optd['submit_qtype']:
+        optd['submit_qtype'] = optd['submit_qtype'].upper()
+    if optd['submit_cluster'] and not optd['submit_qtype']:


Could we not deprecate the -submit_cluster flag? Not necessarily remove it entirely but add a warning message to the help menu and make it do nothing ...

I agree, but that's something for a separate pull request

fsimkovic · 2018-06-26T08:02:53Z

ample/util/options_processor.py

+    if optd['purge']:
+        purge = optd['purge']
+        # Need to handle ackward compatibility where purge was boolean
+        if purge is True:


You can typecast the bool to int simply by putting int(True)

>>> int(True) 1 >>> int(False) 0

So this entire bit of code should probably be simply

try: opt['purge'] = int(opt['purge']) except ValueError: msg = 'Purge must be specified as an integer, got: {}'.format(optd['purge']) exit_util.exit_error(msg) logger.info('*** Purge mode level %d specified - intermediate files will be deleted ***', optd['purge'])

Nice - I love getting rid of unneeded code!

fsimkovic · 2018-06-26T08:05:07Z

ample/util/options_processor.py

@@ -381,6 +325,16 @@ def process_options(optd):
        msg = "Cannot find theseus executable: {0}".format(optd['theseus_exe'])
        exit_util.exit_error(msg)

+    # SCRWL - we always check for SCRWL as if we are processing QUARK models we want to add sidechains to them
+    if not optd['scwrl_exe']:
+        optd['scwrl_exe'] = 'Scwrl4' + ample_util.EXE_EXT


Maybe make absolute path to binary by setting os.path.join(os.environ['CCP4'], 'bin', 'Scwrl4' + ample_util.EXE_EXT)

fsimkovic · 2018-06-26T08:06:42Z

ample/util/options_processor.py

+    if optd['phaser_rms'] != 'auto':
+        try:
+            phaser_rms = float(optd['phaser_rms'])
+            optd['phaser_rms'] = phaser_rms


Use again the try - except - else statement structure. Assignment is not really part of the try - except clause

fsimkovic · 2018-06-26T08:07:05Z

ample/util/options_processor.py

-    #
+    if optd['use_shelxe']:
+        if optd['mtz_max_resolution'] > mrbump_util.SHELXE_MAX_PERMITTED_RESOLUTION:
+            logger.warn("Disabling use of SHELXE as max resolution of {} is < accepted limit of {}".\


Use ,-seperated handling instead of .format

fsimkovic · 2018-06-26T08:10:02Z

ample/util/options_processor.py

-            logger.info('Making fragments (including homologues)')
+    # Model building programs
+    if optd['refine_rebuild_arpwarp'] or optd['shelxe_rebuild_arpwarp']:
+        if not (os.environ.has_key('warpbin')


AAAAAAAAARRRRGH ... don't ever ever ever use has_key again!!

Only joking, it's deprecated in Python 3.X and not really recommended either. Turn this into 'warpbin' in os.environ.

fsimkovic · 2018-06-26T08:10:31Z

ample/util/options_processor.py

-        logger.info('\nMaking Rosetta Models')
-    else:
-        logger.info('NOT making Rosetta Models')
+            logger.info('Using arpwarp script: {0}'.format(os.path.join(os.environ['warpbin'], "auto_tracing.sh")))


Use ,-seperated handling instead of .format

fsimkovic · 2018-06-26T08:11:41Z

ample/util/tests/test_options_processor.py

+        self.assertFalse(options.d['use_shelxe']) 
+        self.assertFalse(options.d['shelxe_rebuild']) 
+
+        # Check we don't accidently turn it off if above limit


Being picky I'd turn this into a separate test function

Yes, but no ;)

But yes ;-)

Typecast to check for purge option Change use of format to arguments for logger Set path to $CCP4/bin for shipped executables

linucks · 2018-06-26T09:04:51Z

The changes were all sensible, so I've made most of them.

I agree that we need to think better about exiting as it causes an issue as the exit code as it's currently written doesn't have access to any AMPLE variables, so it can't clean up properly. I agree that Exceptions are probably the better way to go and then just have the main routine trap them and be responsible for exiting there. Again something for another day.

fsimkovic · 2018-06-26T09:05:09Z

ample/util/options_processor.py

-        logger.info('*** Purge mode level %d specified - intermediate files will be deleted ***', optd['purge'])
+    try:
+        optd['purge'] = int(optd['purge'])
+    except ValueError:


You need to add a KeyError to the exception in case it's not defined.

fsimkovic · 2018-06-26T09:07:56Z

ample/util/options_processor.py

@@ -412,13 +406,12 @@ def process_mr_options(optd):

    # Model building programs
    if optd['refine_rebuild_arpwarp'] or optd['shelxe_rebuild_arpwarp']:
-        if not (os.environ.has_key('warpbin')
-                and os.path.isfile(os.path.join(os.environ['warpbin'], "auto_tracing.sh"))):
+        if not ('warpbin' in os.environ and os.path.isfile(os.path.join(os.environ['warpbin'], "auto_tracing.sh"))):


Instead of doing the same os.path.join twice, I would re-write this bit like:

if optd['refine_rebuild_arpwarp'] or optd['shelxe_rebuild_arpwarp']: auto_tracing_sh = os.path.join(os.environ['warpbin'], "auto_tracing.sh") if 'warpbin' in os.environ and os.path.isfile(auto_tracing_sh): logger.info('Using arpwarp script: %s', auto_tracing_sh) else: logger.warn('Cannot find arpwarp script! Disabling use of arpwarp.') optd['refine_rebuild_arpwarp'] = False optd['shelxe_rebuild_arpwarp'] = False

Thinking about this, what happens if a user decides not to install arpwarp ... then the warpbin is not even in os.environ ... we need to be careful here

Please include this as well Jens

fsimkovic · 2018-06-26T09:11:23Z

I maintain the belief that doing it now is little effort and it gets done. If this gets added to the infinitely-long TODO list, chances are it will never ever be correctly done. Surely it's not too much effort either to exchange the ample_util.exit_call with a sensible raise Exception statement.

fsimkovic · 2018-06-26T17:12:13Z

Jens there are still two outstanding parts. Once a logger message and one the modified if statement.

fsimkovic · 2018-06-26T17:23:44Z

I've sorted it myself now quickly.

linucks added 2 commits June 25, 2018 12:59

Bugfix and minor cleanup of ResultsSummary code

859eb19

Only run SHELXE if the maximum resolution is better then a specified …

af116d3

…value

linucks changed the title ~~Bugfix and minor cleanup of ResultsSummary code~~ Bugfix and minor cleanup of ResultsSummary code + control SHELXE use depending on resolution Jun 25, 2018

fsimkovic suggested changes Jun 26, 2018

View reviewed changes

Use try/Except/else to set variables.

2102128

Typecast to check for purge option Change use of format to arguments for logger Set path to $CCP4/bin for shipped executables

fsimkovic suggested changes Jun 26, 2018

View reviewed changes

Added KeyError to purge check and changed exit calls to raises

bb63e73

fixed some small bits

27485ce

fsimkovic approved these changes Jun 26, 2018

View reviewed changes

fsimkovic merged commit bd12c21 into rigdenlab:master Jun 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix and minor cleanup of ResultsSummary code + control SHELXE use depending on resolution #52

Bugfix and minor cleanup of ResultsSummary code + control SHELXE use depending on resolution #52

linucks commented Jun 25, 2018

fsimkovic left a comment •

edited

Loading

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

linucks Jun 26, 2018

fsimkovic Jun 26, 2018

linucks Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

linucks Jun 26, 2018

fsimkovic Jun 26, 2018

linucks commented Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic Jun 26, 2018

fsimkovic commented Jun 26, 2018

fsimkovic commented Jun 26, 2018

fsimkovic commented Jun 26, 2018

Bugfix and minor cleanup of ResultsSummary code + control SHELXE use depending on resolution #52

Bugfix and minor cleanup of ResultsSummary code + control SHELXE use depending on resolution #52

Conversation

linucks commented Jun 25, 2018

fsimkovic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linucks commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsimkovic commented Jun 26, 2018

fsimkovic commented Jun 26, 2018

fsimkovic commented Jun 26, 2018

fsimkovic left a comment •

edited

Loading