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

Base unit for spatial frequency #223

Closed
jeffjennings opened this issue Dec 5, 2023 · 2 comments · Fixed by #248
Closed

Base unit for spatial frequency #223

jeffjennings opened this issue Dec 5, 2023 · 2 comments · Fixed by #248

Comments

@jeffjennings
Copy link
Contributor

jeffjennings commented Dec 5, 2023

Is your feature request related to a problem or opportunity? Please describe.
The default spatial frequency unit in MPoL is [klambda]. Nothing necessarily wrong with that, but it can be an unintuitive gotcha for users, and depending on which part of the code they're running, can be confusing to identify. It also necessitates putting factors of 1e3 and 1e-3 in many places, and complicates the handful of routines in the codebase that pull in functions from frank (which expect [lambda]). It would be a bit simpler if the base unit throughout the codebase were updated to [lambda], with only plotting routines modifying units.

Describe the solution you'd like
Change the base unit to [lambda] throughout the codebase and in the docs.

@iancze
Copy link
Collaborator

iancze commented Dec 5, 2023

After conversations with @jeffjennings and @j6626 yesterday, I agree we should do this. I thought klamba made a reasonable base unit for ALMA (most visibilities are 10's to 1,000's). But after a while working with this, I don't think klambda is any more intuitive than lambda in this context and has the actual downside of all those 1e-3 and 1e-3 factors you mentioned.

@jeffjennings
Copy link
Contributor Author

jeffjennings commented Dec 7, 2023

Just noting this will also need to be done in visread (I think only here) since, as in #227, visread functions are being called internally in MPoL.

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 a pull request may close this issue.

2 participants