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

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

Merged
merged 18 commits into from
Apr 19, 2024

Conversation

telegraphic
Copy link
Collaborator

@telegraphic telegraphic commented Aug 10, 2023

  • Added Stokes-I and 'coherence' mode to detect.py
  • Added more telescopes to the sigproc.py and sigproc2.py lookup dictionaries
  • Enabled cuda support in quantize.py and unpack.py

Adding Stokes I and coherence modes, which were in a previous PR that was too difficult to merge
I recall we tackled this a few years ago, perhaps in the ill-fated dcp-merge PR.
@codecov
Copy link

codecov bot commented Aug 10, 2023

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.

❗ 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 ⚠️
python/bifrost/blocks/quantize.py 80.00% 2 Missing ⚠️
python/bifrost/blocks/unpack.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jaycedowell
Copy link
Collaborator

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

@jaycedowell
Copy link
Collaborator

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

@league
Copy link
Collaborator

league commented Apr 18, 2024

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.)

I guess that "broken" else branch was being used. Can claim then that
dtype is optional and keep None there.
@jaycedowell
Copy link
Collaborator

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
Copy link
Collaborator

league commented Apr 19, 2024

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 8c5d258, 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
Copy link
Collaborator

league commented Apr 19, 2024

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.

@jaycedowell jaycedowell merged commit b566729 into master Apr 19, 2024
2 checks passed
@jaycedowell jaycedowell deleted the dcp-sigproc-detect-update branch April 19, 2024 20:50
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

3 participants