Dev/simplify imports#138
Conversation
…urce strains in gravwaves._gws_harmonics_at_evo_fobs()
minor bugfix to add missing factor of 1/dlnf in definition of loud so…
…ces-classic Fixing errors in param_spaces_classic.py I believe the code in question was created to create a user-friendly way of reproducing the NANOGrav 15yr Astro Interpretation paper. It was not the code that we actually used for that analysis. This fix corrects a mistake introduced in an attempt to do "open science".
* Fixing errors in param_spaces_classic.py * Fix bug in sams/components.py --------- Co-authored-by: Shreyas Tiruvaskar <sti50@bohr.canterbury.ac.nz>
|
It appears that previous pull requests have made it into this pull request too... |
|
@astrolamb , some of the changes seem to be different from the PR title. For example, in holodeck/librarian/tests/test_lib_tools__param_space.py around line 272 or so. I'm a little confused about the diff here. |
|
@kayhangultekin yeah I messed up somewhere. Even though I branched from |
|
@kayhangultekin which branch is THE branch to update? Because we have a The purpose of this PR is to clean up imports and avoid circular importing. There'll be conflicts in the future though if we don't have a specific branch upon which we're basing/merging from, whether that is |
|
@astrolamb I'm sure I messed it up due to my lack of knowledge of things git and github. By and large, Any thoughts @davecwright3 ? |
|
@kayhangultekin the |
|
@kayhangultekin I didn't realize that you had done a squash merge. This discards all commit history in the merge, but keeps the contents. This is fine for pull requests, but for long-term branches can cause the issues we're seeing here. Essentially, @astrolamb for now, you should base your feature branches on At the end of the day, it's my fault for not looking closely enough at the original |
|
@astrolamb do you know what is so slow in the holodeck import? The only thing I see in # NOTE: Must load and initialize cosmology before importing other submodules!
import cosmopy # noqa
cosmo = cosmopy.Cosmology(
h=Parameters.HubbleParam, Om0=Parameters.Omega0, Ob0=Parameters.OmegaBaryon,
size=200,
)
del cosmopyand we could just cache that function call. The only modules it imports are |
|
Okay. Is there a way to fix my mess up? |
@davecwright3 yeah that probably is it |
|
Chatting with gemini tells me that we can do the following, which addresses @astrolamb's original issue with @davecwright3's altered strategy, if I understand correctly. In class _CosmoProxy:
def __init__(self):
self._real_cosmo = None
def _get_real_cosmo(self):
if self._real_cosmo is None:
# This is the "Lazy Trigger"
print("Initializing slow cosmopy object...")
import cosmopy
from . import Parameters # Ensure Parameters is available
self._real_cosmo = cosmopy.Cosmology(
h=Parameters.HubbleParam,
Om0=Parameters.Omega0,
Ob0=Parameters.OmegaBaryon,
size=200,
)
return self._real_cosmo
# Forward all attribute lookups to the real object
def __getattr__(self, name):
return getattr(self._get_real_cosmo(), name)
# If the real object is callable, forward that too
def __call__(self, *args, **kwargs):
return self._get_real_cosmo()(*args, **kwargs)
# Create the proxy in the holodeck namespace
cosmo = _CosmoProxy()There are a couple of places ( |
Description
In most of
holodeck's modules, it imported ALL other modules withThis isn't ideal -- especially when only one variable is required from another module. Some egrecious examples I have seen include importing all modules to only require
holo.__version__, and importing specific modules such asutils.pyonly to access its subroutines viaholo.utils. It's inefficients, uses up memory, and its messy.Todos
Notable points that this PR has either accomplished or will accomplish.
Notes
In order to make this work, I have had to change the import to
Semi_Analytical_Models, which will be a change for most users. I had to make this change because it resulted in a circular import, which caused a failure.In the previous version, one could import this class using
This no longer works.
In this new version, I recommend the following usage:
This has the advantage of also being neater.
Status