search

ledatelescope / bifrost

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

Romein Convolution #128

Closed KentJames closed 4 years ago

KentJames commented 5 years ago

This gives the ability to use a romein style convolution on the GPU in Bifrost.

Used heavily in the EPIC Correlator for instance, which runs on the bifrost pipeline.

This should have no merge conflicts from what I can see.

ledatelescope commented 5 years ago

"CI4 data": could be complex, 4-bit integer math. What does this actually stand for if not that?

Do you have benchmark and scaling data for the bifrost implementation?

On 7/3/19 12:50 PM, James Kent wrote:

CI4 data

-- Lincoln J. Greenhill Harvard-Smithsonian CfA Office: 1 617-495-7194 60 Garden St, Mail Stop 42 Cell: 1 650 722-7798 Cambridge, MA 02138 FAX: 1 617-495-7345 greenhill@cfa.harvard.edu Skype: ljgreenhill www.cfa.harvard.edu/~lincoln

jaycedowell commented 5 years ago

Yep, CI4 is complex four-bit integer data.

ledatelescope commented 5 years ago

Is 8-bit real and 8-bit imag. viable in the present code? This would enable the re-quantization step to be skipped.

On 7/3/19 3:05 PM, jaycedowell wrote:

Yep, CI4 is complex four-bit integer data.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ledatelescope/bifrost/pull/128?email_source=notifications&email_token=ACL54MX73VCNOYH7B7ZOPTLP5TZ6XA5CNFSM4H5HYFF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZFM6LA#issuecomment-508219180, or mute the thread https://github.com/notifications/unsubscribe-auth/ACL54MVMFTTKBC766PVX2HDP5TZ6XANCNFSM4H5HYFFQ.

-- Lincoln J. Greenhill Harvard-Smithsonian CfA Office: 1 617-495-7194 60 Garden St, Mail Stop 42 Cell: 1 650 722-7798 Cambridge, MA 02138 FAX: 1 617-495-7345 greenhill@cfa.harvard.edu Skype: ljgreenhill www.cfa.harvard.edu/~lincoln

jaycedowell commented 5 years ago

A couple of other comments:

jaycedowell commented 5 years ago

It should support ci4|8|(16)|(32) and cf(32)|(64) inputs with output to cf(32)|(64). Whether or not all possible options have been tested is another question.

KentJames commented 5 years ago

A couple of other comments:

  • Should the shared memory size by a define stored in the user.mk file?
  • Are there unit tests for the set_positions() and set_kernels() methods?

No I don't believe so, I'll see if I can add these in.

  • Should romein_float still be in there?

Absolutely not. I'll remove these.

jaycedowell commented 5 years ago

Something else occurred to me over the weekend: we should at consider what it would take to merge this and my work on Project Orville together. I need to put it somewhere where you can take a look at it.

jaycedowell commented 5 years ago

The Project Orville imager is now in this fork: https://github.com/jaycedowell/bifrost/tree/wideband-imager

jaycedowell commented 5 years ago

For the record: I'm not sure if I like BF_GPU_SHAREDMEM or GPU_SHAREDMEM. I can probably come to grips with the latter but I don't know about the former. Suggestions?

jaycedowell commented 5 years ago

I think the test coverage is decent now in terms of the different inputs and outputs. The only things left in my mind are to (1) think about how to merge this with Orville and to (2) get someone other than me to look the interface over.

I want to think that (1) is feasible but there are several key difference between romein and orville:

There are probably other differences that I am forgetting right now.

jaycedowell commented 5 years ago

The EPIC meeting today reminded me of a couple of things about the unit tests. 1) The current comparison is done by averaging over all of the elements in the grid and checking that the result is less than 0.1. It might be better to do an element-wise comparison with numpy.testing.all_close() with more appropriate tolerances. 2) This also reminded me that I had some strange results from the cf64 unit tests when I was looking at the individual elements. The difference was larger than what I was expected for cf64 and more like what you get from single precision stuff. Maybe there is a subtle problem with how cf64 is implemented? Or in the test itself?

jaycedowell commented 5 years ago

I'm back to being happy with the Romein unit tests.

Looking at this again I noticed that it also adds a new fft_shift_2d() function. The documentation here is a little light on details about what assumptions are built into it/what the limitations are. Also, it doesn't look like there are unit tests for this function.

jaycedowell commented 5 years ago

I took a closer look at fft_shift_2d() and I am not too crazy about the interface. The two dimensional parameters could be determined from the data shape. The method does not appear to support odd-sized dimensions on the shifted axes as I found from the unit tests.

I wonder if this might not be better done with a map() call instead of a dedicated function/module. It might be worth doing some timing tests to see what kind of performance differences there are between the two implementations.

KentJames commented 5 years ago

Thanks for the exceptional work on the unit tests, been a bit awol getting a paper to completion.

Odd sized dimensions is fairly easy to add. Origin needs to be shifted by one pixel. The fft_shift_2d's interface is pretty dreadful I agree. I'll have a play around and see what can be done.

KentJames commented 4 years ago

The reason I added fft_shift_2d was because I got weird behaviour with bifrosts fft shifting, however after testing the pipeline through with it again I don't see any of that behaviour I remember (my memory is far from infallible ).

So I've nuked it from bifrost. I think that resolves the problem.

jaycedowell commented 4 years ago

It looks like the PyPy version problem is the only thing that is causing the Travis builds to fail.

codecov-io commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@2a155b1). Click here to learn what that means. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #128   +/-   ##
=========================================
  Coverage          ?   58.53%           
=========================================
  Files             ?       65           
  Lines             ?     5117           
  Branches          ?        0           
=========================================
  Hits              ?     2995           
  Misses            ?     2122           
  Partials          ?        0
Impacted Files Coverage Δ
python/bifrost/fft_shift.py 0% <0%> (ø)
python/bifrost/romein.py 0% <0%> (ø)

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 2a155b1...9018f69. Read the comment docs.