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

Clean up freq_range and freq_array handling in UVCal #1361

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

bhazelton
Copy link
Member

@bhazelton bhazelton commented Nov 28, 2023

Description

We should really only have one of freq_array or freq_range defined on a UVCal object in the future (e.g. in version 3.0). This is along the lines of what we decided for time_range vs time_array (see #1306).

This is very clear in v 3.0+ because of the move to the wide_band structure. It's a bit muddier before that because of the support for flag arrays that have a frequency axis in delay-type calibrations.

So this PR just adds deprecation warnings for the clear cases so users can know what's coming in v3.0. To be clear:

  • freq_range should not be set on non-wide-band gain style objects (which have a frequency axis)
  • freq_array and channel_width should not be set on wide-band objects (which do not have a frequency axis)

It also fixes a couple of small bugs I found in testing related to handling of the freq_range parameter in the reorder_freqs and __add__ methods.

Motivation and Context

I stumbled on this while working on improvements to UVCal methods that I'm implementing as part of developing the calH5 format. I thought we should get the deprecation warnings in ASAP to give users as much heads up as possible. But I don't actually expect many users to run into this.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change (documentation changes only)
  • Version change
  • Build or continuous integration change

Checklist:

Breaking change checklist:

  • I have updated the docstrings associated with my change using the numpy docstring format.
  • I have updated the tutorial to reflect my changes (if appropriate).
  • My change includes backwards compatibility and deprecation warnings (if possible).
  • I have added tests to cover my changes.
  • All new and existing tests pass.
  • I have updated the CHANGELOG.

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Merging #1361 (adb4889) into main (c643bfd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1361   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          36       36           
  Lines       20230    20243   +13     
=======================================
+ Hits        20214    20227   +13     
  Misses         16       16           
Files Coverage Δ
pyuvdata/uvcal/calfits.py 100.00% <ø> (ø)
pyuvdata/uvcal/uvcal.py 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c643bfd...adb4889. Read the comment docs.

@bhazelton bhazelton force-pushed the uvcal_freq_range_handling branch 3 times, most recently from 2e8b9d4 to 83edc75 Compare December 7, 2023 21:17
mkolopanis
mkolopanis previously approved these changes Dec 7, 2023
Copy link
Member

@mkolopanis mkolopanis left a comment

Choose a reason for hiding this comment

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

Looks good to me. Don't normally touch the UVCal a lot but having a precedent from the time_range/time_array changes helps.

also fix some bugs discovered in testing this
@bhazelton bhazelton merged commit 2b8f706 into main Dec 8, 2023
53 checks passed
@bhazelton bhazelton deleted the uvcal_freq_range_handling branch December 8, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants