-
Notifications
You must be signed in to change notification settings - Fork 371
Move the pycbc array type and some constants away from lalsuite #5217
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: master
Are you sure you want to change the base?
Move the pycbc array type and some constants away from lalsuite #5217
Conversation
|
You can use |
pycbc/constants.py
Outdated
|
|
||
| # Expose the constants as attributes of the module | ||
|
|
||
| C_SI = get_constant('C_SI') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GarethCabournDavies All constants should be gotten from or derived from astropy where it it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or thinks like PI should just be gotten from say numpy / scipy as appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what this will do 'under the hood' - if I remember right there is some reason (I can't remember exactly what) that lal defines its own PI (and TWOPI) that needs to exact match something on their end. I will see if I can find out why
Or are you saying we should use astropy/numpy/scipy constants as preference over LAL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahnitz While I agree that it is better in general to use astropy for constants to be consistent, both of the main GW collaborations (IGWN+3G (lumping these together)) and LISA, define constants separately from astropy, and sometimes this can matter. (Cosmology choices tend to be the most obvious gotcha, but LISA positioning might also be quite sensitive as well).
I think it would be good to consider a mechanism where astropy is the default, but you should be able to say "I want constants from LAL" or "I want constants from lisa_constants" ... We could also try to persuade both those communities to be more consistent, but I'm not sure that's going to happen ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a look at how to implement that. Maybe the pip install pycbc[igwn]-style mechanism may be useful there?
Note: I found that LISA constants for all the bits I could find that were defined by astropy calls straight through anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahnitz While I agree that it is better in general to use
astropyfor constants to be consistent, both of the main GW collaborations (IGWN+3G (lumping these together)) and LISA, define constants separately fromastropy, and sometimes this can matter. (Cosmology choices tend to be the most obvious gotcha, but LISA positioning might also be quite sensitive as well).I think it would be good to consider a mechanism where
astropyis the default, but you should be able to say "I want constants from LAL" or "I want constants fromlisa_constants" ... We could also try to persuade both those communities to be more consistent, but I'm not sure that's going to happen ..
I'm also thinking about using the latest lisa_constants for Mojito Light in PyCBC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spxiwh I think I largely agree. That makes sense only when the constants are unique / specialized / specific variations (as you say cosmology is a good example or things specific to an actual detector).
There should be a preference here though to get constants from more standard sources when they do exist. There are a lot of cases where the constants aren't even different numerically and in those cases we should just switch to keep the set of nonstandard constants limited.
pycbc/constants.py
Outdated
| ) | ||
| # first, do mappings for constants which the fallback is directly in astropy/numpy | ||
| # All are in SI units | ||
| _FALLBACK_MAPPING = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, almost everything in this mapping is something we should consider just not using lal for. A few of these may require checking (earth_si, I could imagine some specific value was chosen that is not known to different precision). Most of these though don't need to come from lal in any situation I can imagine.
I'd even prefer we don't have fallbacks (at least not without understanding the potential use case a bit more), because that means there could be unexpected behavior if say lal is or is not the source.
One thing which is cumbersome about installing pycbc is the sheer size of the package.
Quite a big proportion of this is in lalsuite, and so this is work to start moving away from
lalfor a few bits.There are two changes here; in the pycbc array's epoch, which used LIGOTimeGPS but is now a float64, this is probably the most invasive change. We also update the constants to have a fallback option if lal is not installed
Standard information about the request
This is an efficiency update (for efficiency in dependencies)
This change affects: the offline search, the live search, inference, PyGRB through the array type
This should change nothing in outputs
This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines
Contents
The main things here is the change of he array epoch to a float64
We also have inclusion of a
pycbc.constantsmodule. When imported, this tries to importlal, and then falls back toastropy/numpyequivalents if needed. I have started to update to use that module rather than lal in a few places, but that work will be ongoing.Testing performed
The array type has existing unit tests, which I believe cover this change
I have added a module to test that the constants are consistent between one another. This does require that
lalis installed when the test itself is performed.