Bugfix that causes warning messages when input exons are full UTR int…

[duartemolha]

…roduced in "ENSCORESW-2545"

Description

The commit here:
74e499a

was trying to correct some discrepancies in the softmasking on non-coding sequences
However, completely non-coding exons have a undefined $ex->coding_region_start resulting in warning messages

Use of uninitialized value in numeric gt (>) at ...EnsEMBL/Transcript.pm

After setting all the sequence to lower case at line 837 $exon_seq = lc($exon_seq)
any exons that does not have a defined coding start

the if statements
if ($ex->coding_region_start($self) > $ex->start()) {
and
if ($ex->coding_region_end($self) < $ex->end()) {
should never be done since both coding_region_start and/or coding_region_end will be undefined if
if (!defined ($ex->coding_region_start($self))) is true

Use case

This bug will output warning messages for completely UTR exons where the optional softmask has been set

for example for gene DDR2 , transcript id ENST00000367921

even though the output softmasking is correct before and after my code change, in the updated code we do not get warning messages such as
Use of uninitialized value in numeric gt (>) at .../ensembl/modules/Bio/EnsEMBL/Transcript.pm line XXX.

Benefits

The change I made makes it so that when checking a complete UTR exon (when soft_masking is requested) is all lowercase, and then the comparisons with coding start and coding end with the start and end of the exon are ignored,

Those comparisons are only done IF $ex->coding_region_start is defined
if (!defined ($ex->coding_region_start($self))) {
$exon_seq = lc($exon_seq);
}else{
if ($ex->coding_region_start($self) > $ex->start()) {
...
}

      if ($ex->coding_region_end($self) < $ex->end()) {
        ...
     }

}
$seq_string .= $exon_seq;

Possible Drawbacks

none that I can see

Testing

No. I have not created tests for this. The current tests available test to see if the boundaries between lowerCase and upperCase match.
For exons that are completely UTR, both the previous code and the new code would make the entire exon sequence lowercase. The only difference is that after my change there is no invalid if comparisons with undefined values and therefore no warning messages.

[coveralls]

Coverage remained the same at 81.467% when pulling 85116e3 on duartemolha:hotfix/Bug_on_softmaskig_issueENSCORESW-2545 into 3eda8b4 on Ensembl:master.

[ens-bwalts]

As long as we are making changes here, it might be nice to flip the if-else case to make this more readable. Make the test if (defined ($ex->coding_region_start($self))) and switch the code in the if and else cases.

[ens-bwalts]

Coding style issues: please add a space between brackets and else, also there's some trailing whitespace.

[mkszuba]

I insist on adding a test case confirming we get no unexpected warnings.

[duartemolha]

You clearly did not had test cases before for any of it. If you did this would have been picked up before. I can create test cases for it ... but I would appreciate a little better manners when making demands when someone is pointing out errors in your code and offering some correction for it. I gain nothing from this. So : Could you please include a test case as well. Much appreciated This is how humans usualy talk to each other. Many thanks Duarte

…

On Wed, 7 Aug 2019, 15:35 Marek Szuba, ***@***.***> wrote: ***@***.**** requested changes on this pull request. I insist on adding a test case confirming we get no unexpected warnings. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#404?email_source=notifications&email_token=AAFQIVN2WXEIDTPQAQED6CDQDLMS5A5CNFSM4II5GDW2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCA3IH5A#pullrequestreview-272008180>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFQIVIXO7VEK364J3HQC6TQDLMS5ANCNFSM4II5GDWQ> .

[duartemolha]

Sure ...striving for higher standards is always welcome... And I did delete the PR template... but I am not sure it said there that you insisted on tests. If I have some time I might make them .. In the meantime you can keep the broken code... because ... standards. Sorry but I don't appreciate rudeness especially when someone if volunteering to help

…

On Wed, 7 Aug 2019, 16:09 Marek Szuba, ***@***.***> wrote: You clearly did not had test cases before for any of it. If you did this would have been picked up before. And it's not the worst thing we have got in the Ensembl code base either! Which is part of the reason why we now at least try to have higher standards. This is how humans usualy talk to each other. They also usually follow instructions such as "provide simple unit tests to test the changes" (there aren't any) or "do not modify code without testing for regression" (may or may not have been done, the part of the PR template pertaining to this has been deleted). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#404?email_source=notifications&email_token=AAFQIVJEIVLXDPOMMSHWR2TQDLQR7A5CNFSM4II5GDW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3YXFDQ#issuecomment-519139982>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFQIVLGR2OL5B2BPK4W6OLQDLQR7ANCNFSM4II5GDWQ> .

[tgrego]

I manually tried the method with ENST00000367921 and could not replicate the error.
A test case exposing the error is required, or a bug report so that it can be properly fixed.

[mkszuba]

Closing due to inactivity. By all means do go ahead and reopen if you can provide a test case which will allow us to replicate the error.