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

acor causes problems and isn't actually needed #185

Open
paulthebaker opened this issue May 16, 2022 · 2 comments
Open

acor causes problems and isn't actually needed #185

paulthebaker opened this issue May 16, 2022 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@paulthebaker
Copy link
Member

@kdolum pointed out on slack:

I ran across this code in enterprise_extensions/model_utils.py:

try:
    import acor
except ImportError:
    from emcee.autocorr import integrated_time as acor

I think this type of thing is always a bad idea. We should have specific requirements (in my opinion emcee, not acor), rather than using whichever module is available. Otherwise we will have results that cannot be duplicated because they depend on the user's environment in complex ways.
This particular code also has a bug. integrated_time is a function, but acoris a module. The later code calls acor.acor, which will a work only if the code above loaded acor. If it loaded emcee, you'll get

AttributeError: 'function' object has no attribute 'acor'

Please could we fix this situation once and for all?

acor is only used in two places. First, it's used in model_utils.ul where it is needed to estimate the UL uncertainty. However, that estimate is known to be wrong. Second, it's used to automatically thin chains in model_utils.odds_ratio. We could easily have the user input their own thinning factor, instead of using acor to decide for them.

I suggest we simply remove the acor function from e_e. This will simplify things by eliminating acor and/or emcee as e_e dependencies.

@paulthebaker paulthebaker added the bug Something isn't working label May 16, 2022
@paulthebaker paulthebaker self-assigned this May 16, 2022
@paulthebaker
Copy link
Member Author

There was also some discussion with @Hazboun6 and @AaronDJohnson that post-processing utilities like model_utils.ul and model_utils.odds_ratio more properly belong in la_forge, not e_e. Should we mark functions like these as deprecated, and eliminate them entirely at some point in the future?

@AaronDJohnson
Copy link
Collaborator

AaronDJohnson commented May 19, 2022

The la_forge dev branch contains an odds_ratio calculation that uses bootstrap rather than the old method of computing uncertainties. Further, this uses emcee and not acor. So in this way, I think this has been solved if we move to la_forge as our only post-processing package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants