reorganize utils

[bhazelton]

Description

As suggested in #1444 , this re-organizes the code in utils.py into a number of separate files in a new utils folder with code separated by functionality.

I'd really like feedback on the naming and organization of this, I made a number of choices to get it done that I'm not totally sure about. Note that I changed the utils.io folder to utils.file_io because flake8 was complaining that the io name shadowed a python builtin. I don't love that name, happy to hear other suggestions.

I added imports for a number of functions directly into the utils namespace to prevent errors in other repos. Some of those I deprecated though because I don't think they belong there long term. Also happy to get feedback on this. I tested against:

• pyradiosky and pyuvsim (both of which I plan to update so I don't have to put more of the helpers code into the utils namespace)
• hera_qm, hera_cal and hera_pspec

Are there others I should check?

Motivation and Context

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:

• I have read the contribution guide.
• My code follows the code style of this project.

Build or continuous integration change checklist:

• If required or optional dependencies have changed (including version numbers), I have updated the readme to reflect this.
• If this is a new CI setup, I have added the associated badge to the readme and to references/make_index.py (if appropriate).

[steven-murray]

This is definitely a step forward, I'd say. I've made a few comments on individual module names that I think could be better, but overall I like the names/locations.

For me, I'd stick with utils.io and just silence the flake8 error. If you really want it not to be io than you could maybe call it iofmts or fileio or just formats.

[bhazelton]

@steven-murray I took another pass at the organization, see what you think now. I got rid of helpers by moving most functions into more specifically named files. The true utils functions that are very general I moved into a tools submodule.

[bhazelton]

Once we settle on the organization I will also need to update the docs.

[kartographer]

Once we settle on the organization I will also need to update the docs.

@bhazelton -- I took a look through and I think things look reasonably sensible, though how the docs are going to render was one of the big outstanding questions I had.

[bhazelton]

For me, I'd stick with utils.io and just silence the flake8 error. If you really want it not to be io than you could maybe call it iofmts or fileio or just formats.

@mkolopanis @kartographer votes?

[kartographer]

For me, I'd stick with utils.io and just silence the flake8 error. If you really want it not to be io than you could maybe call it iofmts or fileio or just formats.

@mkolopanis @kartographer votes?

I would vote fileio -- seems most descriptive of the options

[mkolopanis]

I'm okay adding an exception to shadow io i personally like the name and hate how persnickety python is about naming anyway 😆

[kartographer]

I'm okay adding an exception to shadow io i personally like the name and hate how persnickety python is about naming anyway 😆

Oh, misread the above -- thought we were taking the io option off the table. But I agree.

[bhazelton]

Ok, I changed the utils.file_io to utils.io and I did a little more cleanup.

I also did a fairly major update to the docs that I'd like people to take a look at. I chose to list the functions that are importable directly from pyuvdata.utils twice: once in the docs that describe useful functions that are not tied to a class (including uvcalibrate and apply_uvflag) and again in the Developer Docs section, where they are listed according to their actual module. In the developer docs, I also chose to include private functions but I wrote some text at the top pointing out that they aren't part of the public API so can change without notice.

[steven-murray]

@bhazelton I think for the docs it would be cleaner just to have the utils module documented in its own heading (so basically what you have now under the "Developer Docs", but bringing that to the top level). There just be a "Utility Functions" heading in the sidebar. If I'm looking for a function to do something, I don't think I'd check under Developer Docs -- that heading smells more to me like instructions for developers. Then you can get rid of the duplicate "Utility Functions" under "Useful Functions"

[bhazelton]

@steven-murray that sounds reasonable, but I have a few questions:

• how should I document which functions are available directly from the utils namespace vs only available from the submodules? Or should I not worry about that or deprecate all importing directly from the utils namespace?
• If I move the utility functions docs up to the top level should I continue to list the private functions with the warning at the top? Or should I leave private functions undocumented?

[codecov]

Codecov Report

Attention: Patch coverage is 99.84102% with 4 lines in your changes missing coverage. Please review.

Project coverage is 99.75%. Comparing base (b0a1dbd) to head (cc04487).

Files with missing lines	Patch %	Lines
src/pyuvdata/utils/phase_center_catalog.py	99.19%	2 Missing ⚠️
src/pyuvdata/uvdata/uvdata.py	97.50%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           prep_v3.0    #1450      +/-   ##
=============================================
- Coverage      99.93%   99.75%   -0.19%     
=============================================
  Files             42       61      +19     
  Lines          21238    21359     +121     
=============================================
+ Hits           21224    21306      +82     
- Misses            14       53      +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kartographer]

I think this is looking really good @bhazelton! Just a few small suggestions -- I actually like the split between the "developer" and "general use" utilities, so I'd be in favor of keeping that.

[steven-murray]

@bhazelton I think:

1. I'm not totally sure, but I think it would be ok to have the structure like:

Utilities/
   - top
   - level
   - functions
   - .subpackage/
     - .subpackage.function

And I'd be OK if the top-level functions are re-listed in their respective subpackages.

2. I think I'd list the private functions with the warning.

[mkolopanis]

Well I'm about to not be helpful because I like both steven and karto's suggestions. But from a user point of view, probably best to have functions documented at exactly the level they would be called/imported from.

I'm also of the mind that a

Here be dragons

warning in front of "developer" or "private" utilities is probably worthwhile. Let's most users know to stop reading.

[kartographer]

Well I'm about to not be helpful because I like both steven and karto's suggestions. But from a user point of view, probably best to have functions documented at exactly the level they would be called/imported from.

Is the "simplest" path forward then to push apply_uvflag and uvcalibrate back under utils? I wouldn't be opposed to this (nor putting the "private" functions after a warning).

[bhazelton]

Ok, so I think the remaining questions are:

• should uvcalibrate and apply_flags be inside utils or at the top level?
• If they remain at the top level, what should the doc page on them be called?

[kartographer]

* should `uvcalibrate` and `apply_flags` be inside `utils` or at the top level?

I think I've managed to convince myself that putting them back inside utils is the most sensible option.

[bhazelton]

Ok, I've updated the structure and docs given this conversation. I think it's ready for another look!

[steven-murray]

Looks good to me!

[kartographer]

Looking good! The doctest failure looks to be a minor clean-up issue, but it looks like part of the warnings test falling over is that something is awry with the casacore installation?

[bhazelton]

@kartographer ah, so the casacore problem makes some sense given context from here:

casacore/python-casacore#270

My take is that the conda package works because Peter Williams rebuilt the numpy bindings but that the pypi package doesn't. I'll put the numpy restriction back in for the casa optional dependency.

[bhazelton]

@kartographer Thanks, I fixed the doctest and warnings issues. There's still a codecov issue I don't understand. I can tell that the lines it's claiming are missed are actually hit when I run the tests on my machine.

[kartographer]

Looks good, thanks @bhazelton!