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

Change default to include_conjugates=True in UVData.get_redundancies. Also call it out in the top level docstring #1498

Open
bhazelton opened this issue Nov 12, 2024 · 2 comments
Labels
docs documentation UVData
Milestone

Comments

@bhazelton
Copy link
Member

This recently sent @jnhewitt down a rabbit hole. Conjugates are always included when calling compress_by_redundancy but are excluded by default when calling get_redundancies, so she got different numbers of groups when before and after redundant averaging, which was baffling.

It seems to us that the most likely thing for most users to want from get_redundancies would be to have the conjugates included.

At the very least, this very important switch should be called out in the top-level docstring.

@bhazelton bhazelton added docs documentation UVData labels Nov 12, 2024
@bhazelton bhazelton added this to the Version 3.2 milestone Nov 12, 2024
@jnhewitt
Copy link

Specifying "include_conjugates=True" in get_redundancies only partially solved the problem - there were still more groups before compressing by redundancy than after. I think I tracked it down to the setting of "use_grid_alg." If I left it at its default, get_redundancies reports different numbers of groups before and after the redundant average. Here's some data from testing. The two numbers are the number of groups get_redundancies reports for my test dataset before and after the average.

use_grid_alg=False in both compress_by_redundancy and get_redundancies: 2246=>2246
use_grid_alg=True in both compress_by_redundancy and get_redundancies: 2256=>2256
no specification of use_grid_alt: 2256=>2246

So one possible explanation is that the two methods are using different defaults of use_grid_alg. In any case, you can avoid this problem by setting use_grid_alg in both.

@bhazelton
Copy link
Member Author

We agreed we will change all the defaults to match (with a deprecation period and warnings). We'll move to having both include_conjugates and use_grid_alg default to True.

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

No branches or pull requests

2 participants