-
Notifications
You must be signed in to change notification settings - Fork 9
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
Analytic Beam refactoring #369
Comments
I do not think I understand how @steven-murray suggestion work completely, but two questions ... Will this require defining a new yaml constructor for every new subclass of And here a subclass of |
|
Another useful thing to add to the |
We discussed this on a telecon on 8/15/22. Some thoughts:
|
This is being done in pyuvdata, see RadioAstronomySoftwareGroup/pyuvdata#1383 |
I find the implementation of
AnalyticBeam
extremely restrictive and difficult to use for a number of reasons. I'll try to outline my ideas here, and open up discussion for ideas for improvements.My frustrations lie along two major axes, both of which come from trying to use them in
hera_sim
, in the context of our implementations ofPolyBeam
and co.AnalyticBeam
class gets a keyword which tells it which kind of beam to evaluate (Gaussian or Airy etc.). New beams can inherit fromAnalyticBeam
, but it kinda breaks the way that it is meant to be used. ThePolyBeam
for instance can inherit theAnalyticBeam
, but it is a new beam class entirely, and not specified via a new string option. There's no way for new code outside pyuvsim to "register" new analytic beams. This also means that for instance the simulation config code in pyuvsim is restricted to reading the actualAnalyticBeam
class and the options it contains, rather than being able to read new derived beams as well. This is a problem for us in hera_sim.AnalyticBeam
does not follow theUVBeam
. Many of the methods/attributes inUVBeam
are not available inAnalyticBeam
. Some of these make sense since they're just not applicable for an analytic beam, but others would be helpful. For instance, theNfeeds
attribute makes sense in both contexts, but is not available for theAnalyticBeam
. This means wrapper code that has to be able to deal with both has to make a lot of if/else branches to deal with each, which makes code brittle. As an example, theAnalyticBeam
has noselect()
method.There are a few ways to deal with this. I suggest something like the following API:
In fact, with this kind of API it's VERY easy to add a YAML constructor for the BeamModel, so that you can specify a beam like this in YAML:
The text was updated successfully, but these errors were encountered: