Excel validator #62

FranBonath · 2019-11-04T15:15:51Z

Script to check library submission sheets for:

duplications of indexes in pools
color balance of indexes
wrong indexes (e.g. non-nucleotide letters)
mix of different indexes in one pool
index length
ordered cycle number vs. fragment length average

…ss of library submission sheets. Checks for empty entries, missing values and does basic checks on indexes, like check for double index in pool, color balance and length.

…leaned up code and added comments.

maxulysse · 2019-11-04T15:28:05Z

library_information_validator.py

@@ -0,0 +1,495 @@
+#!/usr/bin/env python
+
+# load libraries


Should you organise library?
I like to sort things ;-)

Sorting is nice, but note that from __future__ import print_function is special and must be the first line in the script.

maxulysse · 2019-11-04T15:29:24Z

library_information_validator.py

+            return(False, warnings_numeric)
+
+    # currently not used
+    #def validate_mol(self, min_mol, max_mol):


How could this part of the code be used?

This is for future implementation of a check on pool molarity. I am waiting for Mattias to do some stuff, so Johannes (or me) can create a database, which is required to know which application requires what molarity.

maxulysse · 2019-11-04T15:34:05Z

You know that you're better at Python than I.
But you requested me, so here are some comments (apart from that, I have no idea what the code is doing, and it looks good)

If I remember well there should be some guidelines for such coding practices.

I would comments about what the functions return, just before the definition of the function instead of at the end.
You should always add a space before # when you comment, or not, but be consistent ;-)
I love sorted lists (when importing libraries or such).

jgruselius · 2019-11-05T12:37:25Z

Nice work Fran. Out of curiosity how this works I looked through your code and left some comments. Sorry for sticking my nose in 😇

jgruselius · 2019-11-05T12:41:57Z

library_information_validator.py

+            min_length = sorted(count_length.keys())[0]
+            index_list_colour = []
+            for index in index_seq:
+                index_colour = index.replace('T','G').replace('A','R').replace('C', 'R')


I'm not 100% sure what you do here (maybe you should add more comments explaining it 😛).. I'm pretty sure on two channel chemistry an A gives signal in both channels, i.e. it has both colors.

You are absolutely right! My bad! I was too concentrated on getting the script to work...

jgruselius · 2019-11-05T12:44:33Z

library_information_validator.py

+                if lev.distance(index.lower(), index_seq[i].lower()) < LibrarySheet.MAX_DISTANCE:
+                    logger.warning('The index sequences {} and {} in pool {}'\
+                    ' display low diversity (only {} nt difference).'\
+                    .format(index,index_seq[i], pool_name, lev.distance(index.lower(), index_seq[i].lower())))


You calculate the same Lev. distance twice if the test returns true, if it's an expensive operation maybe you should store the result from lev.distance in a variable for re-use.

jgruselius · 2019-11-05T12:47:11Z

library_information_validator.py

+        index_count = 1
+        for index in index_seq:
+            # checks that indexes only contain valid letters
+            charRE = re.compile(r'[^ATCGNatcgn\-.]')


The regex doesn't change between iterations so you should probably compile it outside the loop.

I was about to point out that . is a special regex character which means "any character" and that your regex was broken. However it seems that if it's within [^.] then it's a special case where it's not a special character. Anyway, I would escape it to help future people reading your code 😅 (it should still work the same when escaped)

charRE = re.compile(r'[^ATCGNatcgn\-\.]')

jgruselius · 2019-11-05T12:49:00Z

library_information_validator.py

+                for row in index_list_colour:
+                    try:
+                        column.append(row[row_nr])
+                    except IndexError:


Maybe you should motivate why you catch and ignore this error.

Yes, and log - even if it's a debug level log. pass is bad style 😉

ewels

I know you didn't ask for my review, but as others were getting in on the act I couldn't resist taking a look as well..

Some of the code made me go a bit cross-eyed when trying to figure out what you were doing, but generally it looks good - nice work! 👍

I've left a few comments generally whinging about code style and being "Pythonic". None of these are a big deal though, they're just best-practice things. They make life easier for anyone else maintaining your code (typically that person is future-you). People whined at me about this stuff for several years and it was annoying, but now that it's mostly ingrained, it's my time to wine at other people.. 😁

ewels · 2019-11-06T13:32:02Z

library_information_validator.py

@@ -0,0 +1,495 @@
+#!/usr/bin/env python
+
+# load libraries


Sorting is nice, but note that from __future__ import print_function is special and must be the first line in the script.

library_information_validator.py

ewels · 2019-11-06T14:50:23Z

library_information_validator.py

+            warning_low_div, warning_index_length,\
+            warning_index_balance = validator.validate_index(result_index, pool, sindex)
+
+            pool_warning = pool, [warning_low_div, warning_index_length, \


As above, the code clarity here is a little scary. Is there a reason that you can't use a dictionary? eg. something like:

pool_warnings.append({ 'pool': pool, 'warning_low_div': warning_low_div, 'warning_index_length': warning_index_length, 'warning_index_balance': warning_index_balance, 'warnings_cycle': warnings_cycle[i], 'warning_index_mix': warning_index_mix, 'warning_length_comp': warning_length_comp, })

No reason, just inexperience :/

It made a bit more sense as I got further on into the code later. But generally, yes returning a structured response like this is better IMHO.

library_information_validator.py

ewels · 2019-11-06T15:17:29Z

library_information_validator.py

+                for row in index_list_colour:
+                    try:
+                        column.append(row[row_nr])
+                    except IndexError:


Yes, and log - even if it's a debug level log. pass is bad style 😉

library_information_validator.py

ewels · 2020-03-31T15:44:59Z

library_information_validator.py

@@ -57,12 +59,11 @@ def projectID(self):
        if(len(re.findall('P\d+P\d+', plate_id))>0):
            project_id_user = re.findall('P\d+', plate_id)[0]
        else:
-            logger.error(
+            sys.exit(logger.error(


The argument for sys.exit() is usually a number which corresponds to an exit code. 0 is good, anything other than 0 is bad (error) - typically 1 when writing code.

logger.error is probably returning None or something else odd. What you want instead I think is the logger.error call followed by sys.exit(1)...

Actually, I got the idea to do it like this from the python documentation. There is says as follows:

"The optional argument arg can be an integer giving the exit status (defaulting to zero), or another type of object. [...] If another type of object is passed, None is equivalent to passing zero, and any other object is printed to stderr and results in an exit code of 1. In particular, sys.exit("some error message") is a quick way to exit a program when an error occurs."

Right - but this doesn't really change my point. logger.error always returns None (I just checked), which means that you are always exiting with exit status 0 - this is interpreted as exiting successfully.

ps. To clarify: sys.exit("some error message") would exit with a non-zero exit code. It's just sys.exit(logger.error("some error message")) which won't..

:( and I thought I was right for once. But I bow to the master and will change it.

alneberg · 2024-01-16T12:21:09Z

@FranBonath should we close this one?

FranBonath · 2024-01-16T13:01:41Z

shrug
or we update and implement it in the website, so ppl can pre-check their sample sheets.

FranBonath added 2 commits August 12, 2019 09:50

first (yet unfinished) version of a python script to check completene…

11756d4

…ss of library submission sheets. Checks for empty entries, missing values and does basic checks on indexes, like check for double index in pool, color balance and length.

added additional index checks and changes requested by M. Ormestad. C…

60108ef

…leaned up code and added comments.

FranBonath requested review from maxulysse and alneberg November 4, 2019 15:15

maxulysse reviewed Nov 4, 2019

View reviewed changes

jgruselius reviewed Nov 5, 2019

View reviewed changes

ewels reviewed Nov 6, 2019

View reviewed changes

responses to Phil's/Joel's comments, cleanup of script

ea2d4e7

ewels reviewed Mar 31, 2020

View reviewed changes

changed sys.exit() code from logger.error to 1

fdb7896

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Excel validator #62

Excel validator #62

FranBonath commented Nov 4, 2019

maxulysse Nov 4, 2019

ewels Nov 6, 2019

maxulysse Nov 4, 2019

FranBonath Nov 5, 2019

maxulysse commented Nov 4, 2019

jgruselius commented Nov 5, 2019

jgruselius Nov 5, 2019

FranBonath Nov 11, 2019

jgruselius Nov 5, 2019

jgruselius Nov 5, 2019

ewels Nov 6, 2019

jgruselius Nov 5, 2019

ewels Nov 6, 2019

ewels left a comment

ewels Nov 6, 2019

ewels Nov 6, 2019

FranBonath Nov 11, 2019

ewels Nov 11, 2019

ewels Nov 6, 2019

ewels Mar 31, 2020

FranBonath Apr 1, 2020 •

edited by ewels

Loading

ewels Apr 1, 2020

ewels Apr 1, 2020

FranBonath Apr 2, 2020

ewels Apr 2, 2020

alneberg commented Jan 16, 2024

FranBonath commented Jan 16, 2024

Excel validator #62

Are you sure you want to change the base?

Excel validator #62

Conversation

FranBonath commented Nov 4, 2019

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

maxulysse commented Nov 4, 2019

jgruselius commented Nov 5, 2019

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

ewels left a 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

FranBonath Apr 1, 2020 • edited by ewels 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

alneberg commented Jan 16, 2024

FranBonath commented Jan 16, 2024

FranBonath Apr 1, 2020 •

edited by ewels

Loading