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

perf: reuse splines on UVBeam objects #45

Merged
merged 11 commits into from
Feb 16, 2022
Merged

perf: reuse splines on UVBeam objects #45

merged 11 commits into from
Feb 16, 2022

Conversation

steven-murray
Copy link
Contributor

Re-uses splines when using beam_list instead of bm_cube. This should give a healthy speedup.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #45 (e201432) into main (1f24660) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   98.63%   98.66%   +0.03%     
==========================================
  Files           5        5              
  Lines         292      300       +8     
  Branches       58       59       +1     
==========================================
+ Hits          288      296       +8     
  Misses          1        1              
  Partials        3        3              
Impacted Files Coverage Δ
src/vis_cpu/conversions.py 98.97% <100.00%> (+0.11%) ⬆️
src/vis_cpu/vis_cpu.py 97.43% <100.00%> (-0.05%) ⬇️

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 535cd7a...e201432. Read the comment docs.

Copy link
Collaborator

@piyanatk piyanatk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good @steven-murray. I approved.

Copy link
Collaborator

@r-pascua r-pascua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, though I would like to see the test I request. You mentioned that this now depends on a pyuvdata PR that hasn't been merged yet, but I don't quite see how. Could you explain? Is it the check_azza_domain kwarg?

Edit: it looks like some tests are failing due to the number of beam pixels being even, so that should be fixed.

src/vis_cpu/conversions.py Show resolved Hide resolved
Copy link
Collaborator

@piyanatk piyanatk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good @steven-murray . Just one comment that might be unnecessary. I have approved.

src/vis_cpu/conversions.py Show resolved Hide resolved
@steven-murray
Copy link
Contributor Author

Thanks @r-pascua and @piyanatk -- I've made the relevant changes.

To your question, @r-pascua, yes the update is the check_azza_domain parameter, which will become available in pyuvdata soon.

@steven-murray steven-murray merged commit 60e8fd0 into main Feb 16, 2022
@steven-murray steven-murray deleted the reuse-spline branch February 16, 2022 15:01
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.

3 participants