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

mpol.plot.vis_histogram_fig to accept loose visibilities? #219

Open
iancze opened this issue Dec 3, 2023 · 1 comment
Open

mpol.plot.vis_histogram_fig to accept loose visibilities? #219

iancze opened this issue Dec 3, 2023 · 1 comment

Comments

@iancze
Copy link
Collaborator

iancze commented Dec 3, 2023

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 a mpol.datasets.GriddedDataset object as input, which requires the user to make assumptions about the image size via a GridCoords object so that they can use the DataAverager to produce a GriddedDataset. 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 and vv numpy arrays directly, as DataAverager or DirtyImager do? Could also borrow logic from inside DirtyImager 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-zero GriddedDataset cells.

@jeffjennings
Copy link
Contributor

jeffjennings commented Dec 4, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants