Script consolidation - Githubissues

mkolopanis commented 4 years ago

Removes the redundant script lwa_bifrost_DFT.py and adds the DFT functionality to the main script. A bit of formatting for readability. Added the synchronization for the DFT accumulation. closes #13

I removed the TransposeOp. It served no purpose anymore since the transpose appears to happen upstream? It was no longer in the original script and with the changes it was not needed for the DFT one.

See comparison with master in this notebook. The DFT shouldn't match but does now between multiple runs.

script_consolidate.pdf multiple runs of the dft code on the main branch dft_comparison.pdf multiple runs of dft code with this branch. dft_comparison_update.pdf

mkolopanis commented 4 years ago

@KentJames Do you want to take a look to make sure I got the DFT code moved over correctly?

mkolopanis commented 3 years ago

Here's another notebook comparison. I ran the code in a python 3 environment (the 'py3_test' file). script_consolidate (2).pdf

KentJames commented 3 years ago

Sorry I clearly missed this. I’ll take a look.

On 18 Sep 2020, at 04:20, Matthew Kolopanis notifications@github.com wrote:

@KentJames Do you want to take a look to make sure I got the DFT code moved over correctly?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

mkolopanis commented 3 years ago

I think it is new as well probably want to remove it. It most likely was a put in to help figure out some int vs float issues I had.

On Wed, Oct 7, 2020, 11:15 Adam Beardsley notifications@github.com wrote:

@adampbeardsley commented on this pull request.

I've looked it over and the only comment I have is that there is a new print statement. I think it's fine to leave it in, unless it was a diagnostic that we don't normally need. I'll hold off on "approving" in case James wants to have a look. Nice work Matt!

In LWA/LWA_bifrost.py https://github.com/epic-astronomy/LWA_EPIC/pull/14#discussion_r501211316 :

print("shapes: locs {locs}, phases {phases}".format(

locs=locs.shape, phases=phases.shape,

))

I think this print statement is new. Do we want it?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/epic-astronomy/LWA_EPIC/pull/14#pullrequestreview-504140329, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACB7BLOOK5XKKJ4CJKSFZJTSJSV4LANCNFSM4RPKM2TA .

KentJames commented 3 years ago

Everything looks kosher to me. And thanks for going through and commenting everything properly Matt... clearly that fell by the wayside a bit in my rush to write up.

I've been thinking for a while about how we can use these both together.. EPIC for the dense core of antennas, and then EPIC-DFT for any outrigger antennas.. seems like we aren't far off now you've merged the two strands into one whole.

mkolopanis commented 3 years ago

I think I added comments as I figured out what things did :smile: it was educational for me.

Here's a notebook showing the vectorized version of the code I changed produces identical outputs. numpize_for_loops.pdf

And that the images are identical dft_comparison (1).pdf

also this shows images are identical in python 2 and 3 now script_consolidate (3).pdf

To be fair in all the comparison I only opened a single file and looked at all the images it contained. Could be more thorough and compare across all frequencies if there is a desire.

mkolopanis commented 3 years ago

@adampbeardsley do you want to take another look over this after the DFT blessing was bestowed?

epic-astronomy / LWA_EPIC

Script consolidation #14

I've looked it over and the only comment I have is that there is a new print statement. I think it's fine to leave it in, unless it was a diagnostic that we don't normally need. I'll hold off on "approving" in case James wants to have a look. Nice work Matt!