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

SImplifying read_cst_beam #1236

Open
steven-murray opened this issue Dec 8, 2022 · 0 comments
Open

SImplifying read_cst_beam #1236

steven-murray opened this issue Dec 8, 2022 · 0 comments

Comments

@steven-murray
Copy link
Contributor

Having a look through UVData.read_cst_beam I get the impression it could be MUCH simplified. Some things I noticed:

  1. There is a discrepancy between some of the keywords to the function, and the keys expected in the YAML file (eg. "frequency/frequencies" and "reference_impedance/ref_imp").
  2. Exactly the same huge set of parameters is used, copy-pasted 3 times in the function. At the very least, these should be packed up into a dict and unpacked on each call.
  3. There's a bunch of weird handling of over-riding YAML inputs with direct inputs to the function. I think it's debatable whether this should be allowed at all (isn't that allowing bad practice?), but if we do allow it, it would be much much easier to take in the parameters of the function as an explicit dictionary, and just do a dict update (point 1 would have to be fixed, of course). This would remove at least 3 repetitions of naming every single parameter that I can see.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants