Adding detector class for calculating LISA projections and response function #4691
Conversation
pycbc/detector.py
Outdated
| # initialize whether to use gpu; FLR has handles if this cannot be done | ||
| self.use_gpu = use_gpu | ||
|
|
||
| def project_wf(self, hp, hc, lamb, beta, t0=None, use_gpu=None): |
There was a problem hiding this comment.
We should mimic the interface here used for the ground based detectors e.g. try to make them as similar as possible. Also, we want to be a little bit future proof as we'll likely have more than just the faslisa implementation and so will want to be able to switch between approximations / implementations of the response.
acorreia61201
force-pushed
the
lisa-emri-pe
branch
from
ab8a59f to
331503a
Compare
|
The One pending issue regards the usage of |
|
@acorreia61201 You'll want to rebase this PR, get the unittest to pass with your PR, and then demonstrate how this compares to say calling BBHx directly. E.g. can you post an example that reproduces BBHx output but using this class and say starting from IMRPhenomD? |
|
@acorreia61201 Can you do this test: from the typical parameter space (prior) of SMBHB and stellar-mass BBH to compute the overlap between BBHx plugin and this PR? Let's do 1000 draw and plot histogram of overlap (not the match). |
acorreia61201
force-pushed
the
lisa-emri-pe
branch
from
c6c46f8 to
da48da3
Compare
acorreia61201
force-pushed
the
lisa-emri-pe
branch
from
6e43ce0 to
eb73c29
Compare
acorreia61201
force-pushed
the
lisa-emri-pe
branch
from
eb73c29 to
c1123ff
Compare
acorreia61201
force-pushed
the
lisa-emri-pe
branch
from
68b61c7 to
0988e18
Compare
…ixed bug where orbit file was truncated by default init params
…consistent with LIGO detector class
… most recent PyCBC patch (6/4/24)
…rbit file start time
…dit companion (temp pypmc fix, add FLR)
acorreia61201
force-pushed
the
lisa-emri-pe
branch
from
26b2e7b to
bfc0cc7
Compare
pycbc/detector.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| reference_time : float (optional) |
There was a problem hiding this comment.
@acorreia61201 For a base class, you want to make sure it really is the minimal set of things that all implementations must have to work. I think there are few things here that don't necessarily qualify. It might be worth going through and pruning a bit here.
A base class for example shouldn't have options just because a child class might have them. It's the child class's job to implement things that are specific to it.
pycbc/detector.py
Outdated
|
|
||
| def get_gcrs_pos(self, location): | ||
| """ Transforms ICRS frame to GCRS frame | ||
| def preprocess(self, hp, hc, polarization=0, reference_time=None): |
There was a problem hiding this comment.
What does this routine mean for a truly generic implementation?
There was a problem hiding this comment.
Both the FLR and LDC versions had some common routines that needed to be applied to the waveforms before inputting them into the response generation. Mainly, this function is making sure that the signals fit into the orbital data being supplied. The original intent was to circumvent errors both backends throw when the signal extends past the orbital data, but I think this could be a generic way to ensure the waveform can be projected given the orbits.
There are some other functions in here (e.g. polarization, padding, start/ref time caching) that I added just to prevent them from being copied for each implementation, although in the interest of making the base class as generic as possible I could move these to the backend classes.
|
An organizational comment, this might be a good time to turn the module into a package itself as the file is getting quite long. |
|
@acorreia61201 It's fine to have common functionality / methods between the LDC / flr methods, but that's somewhat different than what should be in the abstract base class. You could have them share methods by using an auxiliary class that they both inherit from or a simple function that they both can call. So avoiding code duplication is definitely good. For classes though we also want to make sure there is a clear interface of what must be defined so that people can build against it. |
|
@ahnitz When you say turn the module to a package, do you mean moving the space-based classes to a new file (say, |
|
@acorreia61201 I mean this https://docs.python.org/3/tutorial/modules.html#packages e.g. create a 'detector' folder rather than signle module we have now. Then split up the functions into separate files e.g. ground.py space.py etc. A package also has an init.py which can provide truly global / common functions and also preserve the expected namespace by importing things from the submodules so that nobody has to change their existing code and can still do say (pycbc.detector.Detector("H1")... Does that make sense? |
|
I think that makes sense; it seems simple enough looking at some of the other |
…ributes from base to child classes
…ased detectors/utils into different files
|
@ahnitz @WuShichao I've reworked the structure of the space classes and detector module as suggested. Are there any other checks or changes I should make once the unittests pass? |
|
@acorreia61201 Thanks, I will take a look this week. |
…ule modifications
There was a problem hiding this comment.
@acorreia61201 I think this is good enough to move forward with as a starting point. There will likely need to be revision as we learn more.
I'm approving this, but I'd like to at least see the minimum requirement arguments for the absbaseclass methods be added since those define the broadly compatible API.
As we find what is truly common we can always move more things into the baseclass.
pycbc/detector/space.py
Outdated
| return | ||
|
|
||
| @abstractmethod | ||
| def orbits_init(self): |
There was a problem hiding this comment.
I'm not sure if get_links or orbits_init should be in the base class or not. What does get_links return? For oribts_init, why wouldn't that just be a part of the init of the class itself. It doesn't seem to take any arguments.
The standard arguments and expected return should be specified for the base class methods. Yes, it's not enforced, but it's in part so that people (and future developers) know what the API is supposed to be. Project wave for example should specify the minimum interface that can be expected (but not for example any optional argument or extensions).
There was a problem hiding this comment.
get_links in the child classes returns the GW projected to the six laser links. This would be the intermediary step before calculating the TDI combinations. Initially, I thought it would be useful to have this be common to all classes in case, e.g., someone wanted to homebrew their own TDI combinations. I can see in retrospect, though, that this shouldn't be required. The same is true for orbits_init, although I've left them as methods for now just to keep things more organized.
pycbc/detector/space.py
Outdated
| The time in seconds by which to offset the input waveform if | ||
| apply_offset is True. Default 7365189.431698299. | ||
| """ | ||
| def __init__(self, apply_offset=False, |
There was a problem hiding this comment.
Apply offset / offset are probably not general keywords that anything should use, but more like hacks to modify the orbit definitions slightly. They probably don't belong in the base class component.
There was a problem hiding this comment.
I've moved these to the child classes, and I've settled on making detector_name and reference_time base class args. This is mostly just to be similar to the Detector signature; reference time isn't strictly required (at least, it's not required for LDC or FLR). As you said, it'll probably become more clear what's required as we add more implementations for more detectors.
No description provided.