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

Adding detector class for calculating LISA projections and response function #4691

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

acorreia61201
Copy link
Contributor

No description provided.

# 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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

The LISA_detector class in my most recent commit should be sufficient for calculating LISA TDI channels from radiation frame PyCBC waveforms. I am able to replicate the results from the FastLISAResponse tutorial (shown here), with additional functionality for inputting reference times. By default, the TDI channels are calculated using lisatools.detector.ESAOrbits (a simulated realistic orbital file), but orbital information can be input using the Orbits base class in LISA Analysis Tools. All modifications use the most recent versions of FastLISAResponse (ver. 1.0.5) and LISA Analysis Tools.

One pending issue regards the usage of Orbits.configure, which will throw an error with the most recent version of LISA Analysis Tools. I have submitted a pull request to that repository that should solve the issue, but I am still awaiting a response from their development team. Additionally, I haven't been able to conduct timing tests on GPUs, as the most recent version of FastLISAResponse cannot be built with GPU support.

@acorreia61201 acorreia61201 marked this pull request as ready for review July 11, 2024 20:50
@ahnitz
Copy link
Member

ahnitz commented Jul 11, 2024

@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?

@WuShichao
Copy link
Contributor

WuShichao commented Jul 12, 2024

@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).


Parameters
----------
reference_time : float (optional)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


def get_gcrs_pos(self, location):
""" Transforms ICRS frame to GCRS frame
def preprocess(self, hp, hc, polarization=0, reference_time=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this routine mean for a truly generic implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ahnitz
Copy link
Member

ahnitz commented Nov 14, 2024

An organizational comment, this might be a good time to turn the module into a package itself as the file is getting quite long.

@ahnitz
Copy link
Member

ahnitz commented Nov 15, 2024

@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.

@acorreia61201
Copy link
Contributor Author

@ahnitz When you say turn the module to a package, do you mean moving the space-based classes to a new file (say, detector_space.py)? If so, I could move those common methods into functions that can be called by the backends as needed.

@ahnitz
Copy link
Member

ahnitz commented Nov 15, 2024

@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?

@acorreia61201
Copy link
Contributor Author

I think that makes sense; it seems simple enough looking at some of the other __init__ files. I can set this up once I've restructured the base space class and child classes.

@acorreia61201
Copy link
Contributor Author

@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?

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

Successfully merging this pull request may close these issues.

4 participants