Skip to content
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

Minor changes to Seq integer multiplication methods and tests for inplace addition methods #1735

Merged
merged 6 commits into from
Jul 17, 2018

Conversation

chris-rands
Copy link
Contributor

This pull request relates to pull request #1669 and corresponding issue #1659 where we introduced integer multiplication methods for Seq-like objects, I propose 2 small changes.

  1. A small improvement to the trackback for Seq, UnknownSeq, and MutableSeq multiplication methods when subclassing, here is an example:
>>> from Bio.Seq import Seq
>>> class MySeq(Seq): pass
...
>>> MySeq('A') * 2.0
# TypeError: can't multiply Seq by non-int type # Old behaviour
# TypeError: can't multiply MySeq by non-int type # New behaviour
  1. Adding unit tests for in-place Seq, UnknownSeq, and MutableSeq addition methods. These += operations are available even though the __iadd__ methods are never defined, so I thought they should be tested.

I'd be grateful for review of the changes, any thoughts?

One related point, __imul__ methods are defined but __iadd__ methods are not. Does this matter? Probably we should ideally be consistent, but it is likely inconsequential?

  • I hereby agree to dual licence this and any previous contributions under both
    the Biopython License Agreement AND the BSD 3-Clause License.

  • I have read the CONTRIBUTING.rst file and understand that AppVeyor and
    TravisCI will be used to confirm the Biopython unit tests and flake8 style
    checks pass with these changes.

  • I have added my name to the alphabetical contributors listings in the files
    NEWS.rst and CONTRIB.rst as part of this pull request, am listed
    already, or do not wish to be listed. (This acknowledgement is optional.)

@chris-rands chris-rands changed the title Minor changes to Seq integer multiplication methods and tests for addition methods Minor changes to Seq integer multiplication methods and tests for inplace addition methods Jul 13, 2018
@codecov
Copy link

codecov bot commented Jul 13, 2018

Codecov Report

Merging #1735 into master will decrease coverage by 0.39%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1735     +/-   ##
=========================================
- Coverage   85.57%   85.17%   -0.4%     
=========================================
  Files         331      331             
  Lines       50086    50086             
=========================================
- Hits        42859    42660    -199     
- Misses       7227     7426    +199
Impacted Files Coverage Δ
Bio/Seq.py 98.12% <66.66%> (ø) ⬆️
Bio/Phylo/CDAOIO.py 5.42% <0%> (-80.1%) ⬇️
Bio/Phylo/CDAO.py 42.85% <0%> (-57.15%) ⬇️
Bio/Phylo/_cdao_owl.py 88.88% <0%> (-11.12%) ⬇️
BioSQL/DBUtils.py 68.49% <0%> (-6.85%) ⬇️
Bio/Phylo/_io.py 91.66% <0%> (-2.78%) ⬇️
Bio/_py3k/__init__.py 75% <0%> (-1.2%) ⬇️
Bio/Restriction/Restriction.py 82.93% <0%> (-0.46%) ⬇️
Bio/PopGen/GenePop/Controller.py 64.47% <0%> (-0.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7789a5...7e88c3b. Read the comment docs.

@peterjc
Copy link
Member

peterjc commented Jul 15, 2018

Including the class name in the exception is a nice improvement in case of subclasses. Likewise the extra tests seems sensible too.

The TravisCI failures seem unrelated, Python 2.7.14,

======================================================================
FAIL: test_partial_diagram (test_GenomeDiagram.DiagramTest)
Construct and draw SVG and PDF for just part of a SeqRecord.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/biopython/biopython/Tests/test_GenomeDiagram.py", line 770, in test_partial_diagram
    == gdd.write_to_string('PDF').replace(b"\r\n", b"\n")
AssertionError
----------------------------------------------------------------------

and Python 3.5,

======================================================================
FAIL: test_partial_diagram (test_GenomeDiagram.DiagramTest)
Construct and draw SVG and PDF for just part of a SeqRecord.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/biopython/biopython/Tests/test_GenomeDiagram.py", line 770, in test_partial_diagram
    == gdd.write_to_string('PDF').replace(b"\r\n", b"\n")
AssertionError
----------------------------------------------------------------------

My guess is the release of ReportLab 3.5.0 earlier this week is to blame, see https://pypi.org/project/reportlab/#history

In this case the TravisCI weekly cron job will test the master branch in a few days - and I expect that to fail.

After confirming that, your changes look good to merge. Do you want to add yourself to the NEWS.rst file for the next release?

@chris-rands
Copy link
Contributor Author

Thank you Peter, yes, I added myself to NEWS.rst. I saw the TravisCI failure but could not interpret it, your guess sounds very plausible. Let me know if there is anything else you need from me

@peterjc peterjc merged commit 74c92f0 into biopython:master Jul 17, 2018
@chris-rands chris-rands deleted the Seq_mul_add_fixes branch July 17, 2018 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants