HERA-Team / hera_pspec

HERA power spectrum estimation code and data formats
http://hera-pspec.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
5 stars 3 forks source link

Fix api call to ENU_from_ECEF for pyuvdata 3.0 #402

Closed jsdillon closed 1 month ago

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.09%. Comparing base (cd12a55) to head (aaaab5e).

Files Patch % Lines
hera_pspec/pspecdata.py 92.85% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #402 +/- ## ========================================== - Coverage 96.17% 96.09% -0.08% ========================================== Files 17 17 Lines 6118 6121 +3 ========================================== - Hits 5884 5882 -2 - Misses 234 239 +5 ``` | [Flag](https://app.codecov.io/gh/HERA-Team/hera_pspec/pull/402/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_pspec/pull/402/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=HERA-Team) | `96.09% <95.23%> (-0.08%)` | :arrow_down: | 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.

review-notebook-app[bot] commented 1 month ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

steven-murray commented 1 month ago

@jsdillon I fixed all the issues except three things:

  1. There are a bunch of errors on the setup_class() method of Test_Utils. However, the line on which it errors does not exist on my local copy (nor if you look at the "Changes" in this PR). I cannot understand where this code is coming from that is causing the error. Is the github action checking out a different commit than the one in this PR??
  2. Python 3.9 fails because there is no pyuvdata v3 for it. I could update the one breaking line of code in this PR so that it can work also with pre-pyuvdata 3, but in all other repos I've gone with "just require pyuvdata 3", so I'd argue for doing this, and support python 3.10+
  3. There are two more errors occurring now in window function tests. I wonder if @adeliegorce could weigh in on these.
jsdillon commented 1 month ago

1) No clue.

2) Agreed, drop python 3.9.

jsdillon commented 1 month ago

I removed some irrelevant test and added python 3.12 testing, but now one WF test is failing again if you want to look at it @adeliegorce.

I'm not so worried about the coverage issue, but maybe @steven-murray has an idea of why pyuvdata changes have changed the coverage here?

adeliegorce commented 1 month ago

Hi @jsdillon, I fixed the issues in the UVWindow tests, we're just down to the coverage issue (it really is just one rather long if statement to cover)

steven-murray commented 1 month ago

I totally don't think it's worth trying to cover that one corner case exception.

jsdillon commented 1 month ago

Agreed. I'll merge.

adeliegorce commented 1 month ago

Ah but it brought the coverage way down…

steven-murray commented 1 month ago

Not according to codecov? It was just one line

adeliegorce commented 1 month ago

I thought I saw somewhere coverage was down to 92% but you're right, it's actually still around 96%