Skip to content

Restructure package#21

Merged
aknierim merged 19 commits intomainfrom
restructure
Sep 24, 2025
Merged

Restructure package#21
aknierim merged 19 commits intomainfrom
restructure

Conversation

@aknierim
Copy link
Member

@aknierim aknierim commented Aug 25, 2025

  • Restructure package to be in line with our other packages
  • Expose pyvisgrid.core.gridder.Gridder class as a plugin compatible with pyvisgen, see also Add plugin support to pyvisgen pyvisgen#108
  • Make casatools optional dependency (casa optional dependency group)
  • Add docs

@aknierim aknierim requested review from Kevin2 and tgross03 August 25, 2025 13:40
@aknierim aknierim added the enhancement New feature or request label Aug 25, 2025
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 34.39%. Comparing base (098bd9e) to head (5c0a316).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   32.82%   34.39%   +1.57%     
==========================================
  Files           6        8       +2     
  Lines         390      407      +17     
  Branches       70       70              
==========================================
+ Hits          128      140      +12     
- Misses        258      263       +5     
  Partials        4        4              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@tgross03 tgross03 left a comment

Choose a reason for hiding this comment

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

Is there a reason for the removal of casatools as a default dependency? As Measurement Sets are the standard for many of the largest observatories (ALMA, VLA, MOJAVE / VLBA etc.), we should be supporting it by default.

@aknierim
Copy link
Member Author

aknierim commented Aug 25, 2025

I don't see the point of having casatools a dependency and not an optional dependency if it is used in a single method. This is why I included the try except block here:

try:
from casatools.table import table
CASA_AVAIL = True
except ModuleNotFoundError:
CASA_AVAIL = False

This checks whether casatools is available and raises a ModuleNotFoundError later on if the user has not installed casatools but tries to access the Gridder.ms classmethod:

if not CASA_AVAIL:
raise ModuleNotFoundError(
"Cannot import casatools. Please make sure "
"you installed pyvisgrid with the optional "
"casa dependency (uv pip install 'pyvisgrid[casa]')!"
)

If users want casa support, they can just install the casa extra. This is something that has to be added to the docs, of course.

@tgross03
Copy link
Member

I don't see the point of having casatools a dependency and not an optional dependency if it is used in a single method. This is why I included the try except block here:

The gridding of measurement sets is a central feature of the package since it is one of the most common ways to save visibility data. I think that we shouldn't use optional dependencies if important features need the contained packages. In case of something like radiotools it is sensible to include casatools as optional since the package contains many modules which do not need it. But in this case one of the three main methods to use for gridding requires it.
Since we want our software to be accessible and easy-to-use for people outside of the radionets package framework, it is better to avoid additional installation steps to enable a core feature of the Gridding.

@aknierim
Copy link
Member Author

Additional installation steps? Give me a break.
Please tell me where

$ pip install pyvisgrid[casa]

is an additional installation step.

It simply does not make sense for a package that is used in exactly one single method, and that restricts the python version compatibility, to be a dependency if we could just add it as an extra. Besides, the main reason pyvisgrid exists is still pyvisgen.

@tgross03
Copy link
Member

Additional installation steps?

The base package installation does not include it, which means that prior knowledge of the additional package dependency group casa is required. One could argue about whether this qualifies as an "installation step", but for external users it likely does feel like extra effort compared to a simple pip install pyvisgrid, since they also need to know that casa is not included by default even though it’s a central feature.

It simply does not make sense for a package that is used in exactly one single method, and that restricts the python version compatibility

On the "one method" point, I think it's a bit broader: The Gridder class has four non-internal methods beyond plotting and three of those are classmethods, of which one is for casa.
After testing it seems that casatools is also working with python=3.11 and python=3.12 at minimum, which means that the compatibility for higher versions is not restricted by it.

@aknierim
Copy link
Member Author

aknierim commented Aug 26, 2025

The base package installation does not include it, which means that prior knowledge of the additional package dependency group casa is required.

That is exactly what the docs and the README are for. Again, this is something you explain in the docs, so users can choose what version to install with what extras. And adding [casa] to an install command you can also copy from the docs is no effort.

On the "one method" point, I think it's a bit broader: The Gridder class has four non-internal methods beyond plotting and three of those are classmethods, of which one is for casa.

And two of which are not. So I do not see your point here.

After testing it seems that casatools is also working with python=3.11 and python=3.12 at minimum, which means that the compatibility for higher versions is not restricted by it.

Yes, but for py313, for example, it is not. So it is restrictive.

@aknierim aknierim merged commit 2506995 into main Sep 24, 2025
6 checks passed
@aknierim aknierim deleted the restructure branch September 24, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants