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

Automate fixing common beam failures #81

Merged
merged 26 commits into from Aug 21, 2020

Conversation

e-koch
Copy link
Collaborator

@e-koch e-koch commented Jan 29, 2020

We rely on an approximate common beam solver that sometimes fails when the solution is marginally (but within the tolerance) smaller than the true common beam. To encourage the solution to converge to an ellipse just marginally larger (<<1% in area) but can be deconvolved from all beams in the set, we add an epsilon to the perimeter points used in the solver. Certain cases still fail when epsilon is too small.

This PR allows epsilon to iteratively increase up to some set maximum to (hopefully) automate what the user would do when this error comes up.

  • Add explanation to docs.

@e-koch
Copy link
Collaborator Author

e-koch commented Jan 29, 2020

  • update testing framework

@e-koch
Copy link
Collaborator Author

e-koch commented Aug 20, 2020

A bit more detail: the epsilon we increase the edges of the beams to use in the common beam calc. will lead to overestimating the beam area by up to (1+epsilon)^2. That should always negligible when the beam is sampled by 3-5 pixels and epsilon<1e-1 (default is 5e-4).

Increasing epsilon occurs iteratively when the common beam solution cannot be deconvolved from all beams in the sets. This is controlled by

  • auto_increase_epsilon=True -- enables iteratively recalculating the common beam until it works
  • epsilon=5e-4 -- starting value of epsilon
  • max_iter=10 -- max. number of iterations to try
  • max_epsilon=1e-3 -- Maximum epsilon to try.

For each iteration, we just re-do the solution after increasing epsilon up to max_epsilon=1e-3 by default over a maximum max_iter=10. There's probably a more efficient way of doing this, but this is does work without rewriting large chunks of the common beam code.

The default values work well for the cubes I was having issues with but may still not work for certain beam sets. I've added notes in the docs about this and further steps to solve the problem. We could increase the default max_epsilon to ~1e-2 to be more lenient.

@e-koch e-koch requested a review from keflavich August 20, 2020 22:46
@e-koch e-koch requested a review from low-sky August 20, 2020 22:49
@keflavich
Copy link
Contributor

@e-koch could you highlight the part that needs review? I assume the infrastructure stuff doesn't; is it just a question of the epsilon parameter?

@e-koch
Copy link
Collaborator Author

e-koch commented Aug 21, 2020

@keflavich -- Yeah, it's just treating epsilon that's been changed: https://github.com/radio-astro-tools/radio-beam/pull/81/files#diff-2448467c71a5203d77d911cb9b0cce26R480. And the docs description: https://github.com/radio-astro-tools/radio-beam/pull/81/files#diff-5ce429315213083e5243a4a1c82aa776R27.

Apart from that, it's how lenient we should let the default max_epsilon be (it's max_epsilon=1e-3 now). I've set fairly strict defaults but increasing the default should avoid most common beam failures.

I'm not sure why the infrastructure changes are showing up here... I already merged #82 with those commits. Will check.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

Alright, this looks good to me. Do check on why the infrastructure stuff is showing up here, but otherwise I approve


center, radii, rotation = \
getMinVolEllipse(edge_pts, tolerance=tolerance)
if pa.value == -np.pi or pa.value == np.pi:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be pa = -np.pi * u.rad so that a PA in degrees will catch on this too?

Copy link
Contributor

Choose a reason for hiding this comment

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

also is == OK here, or do we need almost_equal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

== is ok since pa uses np.arctan2 and has the exact angles defined. And small angles from 0. deg don't really affect the beam properties.

docs/install.rst Show resolved Hide resolved
@e-koch
Copy link
Collaborator Author

e-koch commented Aug 21, 2020

@adamginsburg Still unsure why the infrastructure commits are showing up in this PR. But a local merge worked as expected without duplicate commits so I'll give it a shot here.

@e-koch e-koch merged commit 97b2118 into radio-astro-tools:master Aug 21, 2020
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

Successfully merging this pull request may close these issues.

None yet

3 participants