ledatelescope / bifrost

A stream processing framework for high-throughput applications.
BSD 3-Clause "New" or "Revised" License
66 stars 29 forks source link

Updates to detect.py, sigproc.py and quantize.py blocks #218

Closed telegraphic closed 5 months ago

telegraphic commented 1 year ago
codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 42.10526% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 67.50%. Comparing base (f5b0b02) to head (5853bc1). Report is 32 commits behind head on master.

:exclamation: Current head 5853bc1 differs from pull request most recent head 1c074b7. Consider uploading reports for the commit 1c074b7 to get more accurate results

Files Patch % Lines
python/bifrost/blocks/detect.py 0.00% 8 Missing :warning:
python/bifrost/blocks/quantize.py 80.00% 2 Missing :warning:
python/bifrost/blocks/unpack.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #218 +/- ## ========================================== - Coverage 67.56% 67.50% -0.06% ========================================== Files 65 65 Lines 5515 5524 +9 ========================================== + Hits 3726 3729 +3 - Misses 1789 1795 +6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jaycedowell commented 1 year ago

@league can you take a look at the type hints in blocks/quantize.py?

jaycedowell commented 6 months ago

@league can you take a look at the type hints in the PR?

league commented 5 months ago

Looking at and validating type annotations. One error I don't know what to do with yet... the comment in sigproc2.py that says "this is broken" ... the code just beneath fails validation because self.dtype should not be None. Would it be appropriate to throw an exception instead?

There's another error about BufferedIOBase.readinto on line 365. For our reference, I'm running the checker as

mypy --follow-imports=silent bifrost/blocks/{detect.py,quantize.py,unpack.py} bifrost/sigproc*.py

to focus just on the files touched by this PR. (With the default follow-imports setting, there are lots of error that show up in imports, including in libbifrost_generated, so using follow-imports=skip or =silent is appropriate to gradually introduce type checking to a project.)

jaycedowell commented 5 months ago

Looking at and validating type annotations. One error I don't know what to do with yet... the comment in sigproc2.py that says "this is broken" ... the code just beneath fails validation because self.dtype should not be None. Would it be appropriate to throw an exception instead?

I think the correct thing to do is throw an exception and let the user know that sub-byte types are currently unsupported.

league commented 5 months ago

I think the correct thing to do is throw an exception and let the user know that sub-byte types are currently unsupported.

I tried it in 8c5d258c77636d4784bf8b639bedc481490975f3, and it turned out that one of the test cases failed due to the exception…

class TestPrintHeader(unittest.TestCase):
    """Test all aspects of the print header block"""
    def setUp(self):
        self.fil_file = "./data/2chan4bitNoDM.fil"
    def test_read_sigproc(self):
        ...

Apparently sigproc can proceed with self.dtype = None because elsewhere there's a similar condition (nbits < 8) that avoids using the dtype. So I set its type to Optional and both type-checker and tests are happy again.

league commented 5 months ago

With respect to type annotations in the files touched by this PR, I think they are ready to go. I added a "make check" to the Makefile that runs the mypy checker on a limited selection of files. The idea could be that we gradually expand its scope. It's not run by any CI actions, but maybe that could come later as well.