Open iancze opened 9 months ago
It's nice to make utility functions more flexible, though in this case I think it's a bit overkill. It is just a plotting function, the coarse and fine 1d histograms produced by it with the gridded data represent the visibility distribution well enough for practical purposes, and if a user doesn't know their preferred image size (which they really should), a simple conversion of min(np.hypot(u, v))
, or max
for pixel size, would suffice. utils.get_optimal_image_properties
also gives a sensible pixel size given a desired image width and the (u,v) points.
I think it's useful and convenient to keep support for a gridded dataset as input, and the simplified logic of switching wouuld amount to removing one line (with more added to handle loose vis). I think a simpler solution is a small plotting function that shows the 2d (u,v) distribution, #221
Is your feature request related to a problem or opportunity? Please describe. The new
mpol.plot.vis_histogram_fig
routine is great! But it requires ampol.datasets.GriddedDataset
object as input, which requires the user to make assumptions about the image size via aGridCoords
object so that they can use theDataAverager
to produce aGriddedDataset
. They're probably going to want to do this at somepoint anyway, but it seems a bit premature to make them do this while they're still familiarising themselves with the visibility dataset.The current docs also have some
unit=:math:[klambda]
that didn't render.Describe the solution you'd like Could
vis_histogram_fig
accept (non-Hermitian pairs)uu
andvv
numpy arrays directly, asDataAverager
orDirtyImager
do? Could also borrow logic from insideDirtyImager
to expand the arrays so that they properly include Hermitian pairs in the histogram.And then the logic inside
vis_histogram_fig
could be simplified slightly since we don't need to worry about indexing zero or non-zeroGriddedDataset
cells.