HERA-Team / hera_sim

Simple simulation code for HERA-like redundant interferometric arrays
Other
16 stars 8 forks source link

fix: import blackmanharris from scipy.signal.windows #305

Closed steven-murray closed 5 months ago

steven-murray commented 6 months ago

Changes import location for blackman-harris

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 95.23810% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.43%. Comparing base (8a836c8) to head (921110a). Report is 1 commits behind head on main.

Files Patch % Lines
hera_sim/cli_utils.py 50.00% 0 Missing and 1 partial :warning:
hera_sim/simulate.py 96.96% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #305 +/- ## ========================================== + Coverage 93.02% 93.43% +0.41% ========================================== Files 24 24 Lines 3224 3243 +19 Branches 705 711 +6 ========================================== + Hits 2999 3030 +31 + Misses 124 117 -7 + Partials 101 96 -5 ``` | [Flag](https://app.codecov.io/gh/HERA-Team/hera_sim/pull/305/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HERA-Team) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/HERA-Team/hera_sim/pull/305/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HERA-Team) | `93.43% <95.23%> (+0.44%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HERA-Team#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bhazelton commented 5 months ago

This would be nice to get in, it's causing failures in a lot of CI runs on RASG repos.

r-pascua commented 5 months ago

Sorry this is taking a while to get in @bhazelton! This is currently stalling on updating things to play nice with the new random number generation API. I had a bit of a misconception that the legacy API of seeding the random state was no longer supported, but it seems like that functionality is technically still supported (even if it's not best practice). @steven-murray, if you're OK with it, then I would like to suggest that we revert any changes that were made to random number generation, get this PR merged, and start a new PR for updating the way we handle random numbers.

steven-murray commented 5 months ago

@r-pascua it depends on why the RASG runs are failing. If they're failing on warnings tests, then updating the RNG stuff is necessary, because that's most of the warnings (I think). @bhazelton can you comment as to what in particular is failing? If the RNG stuff is not necessary to solve here, we can punt to another PR.

bhazelton commented 5 months ago

It's not warnings. It's errors, specifically: ImportError: cannot import name 'blackmanharris' from 'scipy.signal' (/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/scipy/signal/__init__.py) Here's a recent example: https://github.com/RadioAstronomySoftwareGroup/pyuvsim/actions/runs/8740622498/job/23984712061?pr=461

It's causing both hera_sim tests and hera_cal tests to fail on pyuvdata & pyuvsim.

steven-murray commented 5 months ago

@bhazelton yep I see that. OK, I rolled back changes to the RNG, so we can merge this in more quickly.

r-pascua commented 5 months ago

OK, so I looked into the failing test a bit more, and it looks like what's happening is that the test is stalling when it tries to run the UVSim simulation to compare against matvis. Oddly enough, the same section runs just fine on the macos build for the same Python version. I consulted with @bhazelton and she said that this is an issue that hasn't been seen before, but pyuvsim has officially dropped support for Python 3.9, and the stalling was a known issue that had to do with MPI synchronization issues (the unseen thing was that the same thing that stalls on Ubuntu runs just fine on macos).

@steven-murray I'm not quite sure what to do here. The simplest option is to drop support for Python 3.9, but that feels a little unsatisfying. That said, I don't have a great idea of how to go about finding a resolution to the issue, and I figure it's not the best idea to try to support something that isn't supported by the dependency that's breaking.

Curious to hear your thoughts.

steven-murray commented 5 months ago

@r-pascua I'm totally fine with dropping support for 3.9. This code has a limited user-base, and it's not hard to upgrade python versions these days.