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

fix: update VisCPU simulator wrapper to match vis_cpu v1 #233

Merged
merged 15 commits into from
Aug 16, 2022

Conversation

steven-murray
Copy link
Contributor

The use_pixel_beams and bm_pix options are no longer available. use_pixel_beam is always effectively False. This has been deprecated already for a while.

One upshot of this is that some of the beam tests no longer work -- some of them were comparing a pixel beam sim to a non-pixel-beam sim. I don't think this was a particularly good strategy for testing the beams anyway, and I've removed the code. But we should probably come up with something to replace those kinds of tests.

BREAKING CHANGE: the use_pixel_beams and bm_pix options
  are no longer available. `use_pixel_beam` is always
  effectively False. This has been deprecated already
  for a while.
@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #233 (27ea32d) into main (fda05d9) will increase coverage by 0.00%.
The diff coverage is 98.30%.

@@           Coverage Diff           @@
##             main     #233   +/-   ##
=======================================
  Coverage   96.56%   96.56%           
=======================================
  Files          24       24           
  Lines        2849     2822   -27     
=======================================
- Hits         2751     2725   -26     
+ Misses         98       97    -1     
Impacted Files Coverage Δ
hera_sim/antpos.py 100.00% <ø> (ø)
hera_sim/visibilities/simulators.py 85.46% <94.44%> (-1.81%) ⬇️
hera_sim/visibilities/vis_cpu.py 92.90% <96.55%> (-0.93%) ⬇️
hera_sim/__init__.py 100.00% <100.00%> (+6.45%) ⬆️
hera_sim/__yaml_constructors.py 94.59% <100.00%> (-0.15%) ⬇️
hera_sim/adjustment.py 99.23% <100.00%> (ø)
hera_sim/beams.py 98.22% <100.00%> (ø)
hera_sim/cli_utils.py 98.78% <100.00%> (ø)
hera_sim/components.py 91.83% <100.00%> (-0.09%) ⬇️
hera_sim/defaults.py 95.53% <100.00%> (ø)
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

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

From what I can tell, this all looks good! When you get a chance, maybe delete the commented out stuff in the tests. On that note, it's probably worthwhile for us to brainstorm a bit about what the purpose of the beam-related tests was and whether it's appropriate to be testing that here (i.e. whether we should bother rethinking and rewriting the tests).

Thanks for doing this!

P.S. In the future, if we're adding some new cool features (like isort), then can we keep that to a single PR? Having the whole code base be reformatted is pretty noisy and makes it a little difficult to figure out what was just reformatting and what was genuinely changed as part of the PR.

hera_sim/__init__.py Show resolved Hide resolved
hera_sim/components.py Show resolved Hide resolved
hera_sim/tests/test_beams.py Outdated Show resolved Hide resolved
hera_sim/tests/test_beams.py Outdated Show resolved Hide resolved
hera_sim/tests/test_beams.py Outdated Show resolved Hide resolved
hera_sim/tests/test_beams.py Outdated Show resolved Hide resolved
hera_sim/tests/test_beams.py Outdated Show resolved Hide resolved
hera_sim/tests/test_beams.py Outdated Show resolved Hide resolved
hera_sim/tests/test_beams.py Outdated Show resolved Hide resolved
hera_sim/visibilities/vis_cpu.py Show resolved Hide resolved
@steven-murray
Copy link
Contributor Author

Thanks @r-pascua! Good call on all the commented code -- I've removed it.

And yeah, sorry about the isort thing. That was a brain fart.

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

2 participants