Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
==========================================
- Coverage 44.91% 44.74% -0.18%
==========================================
Files 87 87
Lines 6773 6810 +37
Branches 1099 1103 +4
==========================================
+ Hits 3042 3047 +5
- Misses 3458 3490 +32
Partials 273 273 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
moble
left a comment
There was a problem hiding this comment.
In addition to the individual comments:
- Tests needed, at least to make sure the output is sensible
sxs/plugins/needs an__init__.py.
I now realize there are several places that seem to assume astropy(?) input types. We need to make sure that those inputs really have the expected types. (I think we're supposed to use .to_value(unit) instead of just .value?) In particular, that assumption shouldn't make it into the sxs code; it should just be in the plugin code, since sxs doesn't know about astropy, etc.
| return type(self)(data, **self._metadata.copy()) No newline at end of file | ||
| return type(self)(data, **self._metadata.copy()) | ||
|
|
||
| def preprocess(self, t1=0, t2=500, t3=None, t4=None, dt=None, **kwargs): |
There was a problem hiding this comment.
A few things:
First, we need to figure out a better way to do this to minimize code duplication. There are several ways this might work. One possibility would be to just move the function here, and guard the two changes with a test for the type of self — possibly making the WaveformModes version call this one with different arguments. The other would be to extract a lot of the code into a helper function somewhere else, and just make the two preprocess methods call that code with their own specializations.
Second, it's not clear to me that other people will want our preprocessing step at all; it's rather specific to how I want to do preprocessing, including the specific parameters for the transition window(s) and the line-subtraction step to remove memory discontinuities.
Third, note that the defaults include t1=0, t2=500. When this is applied to data that's gone through to_lvk, I think the t=0 point moves to the peak of the waveform, so this zeros out everything before the peak, and is still modifying the data far into the ringdown.
| ##Fourier transform | ||
| h = WaveformModes(time = t, | ||
| input_array = h_array, | ||
| modes_axis = 1, |
There was a problem hiding this comment.
This should usually be 0. I think you don't need this argument at all.
| return result | ||
| def fourier_transform(self, total_mass=None, luminosity_distance=None): |
There was a problem hiding this comment.
Needs a space:
| return result | |
| def fourier_transform(self, total_mass=None, luminosity_distance=None): | |
| return result | |
| def fourier_transform(self, total_mass=None, luminosity_distance=None): |
| h_tilde : TimeSeries | ||
| Unitful Fourier transform. | ||
| """ | ||
| from . import m_sun_in_meters, m_sun_in_seconds, parsec_in_meters |
There was a problem hiding this comment.
It might be worth nitpicking about LISA's conventions compared to mine:
Lines 49 to 61 in b7d25d5
| result = result.pad(pad_length, mode=pad_mode, **kwargs) | ||
| result = result.line_subtraction(treat_as_zero=treat_as_zero) | ||
| return result | ||
| def fourier_transform(self, total_mass=None, luminosity_distance=None): |
There was a problem hiding this comment.
This will mutate the data in place. Elsewhere, this is fine because where it's called from is right behind an evaluate call. But here, it could bite people.
| h_unitful *= h_factor.value | ||
| h_unitful.t *= t_factor.value |
| if luminosity_distance is not None: | ||
| h_factor /= (luminosity_distance * 1e6 * parsec_in_meters) | ||
| h_unitful = self | ||
| if total_mass is not None and luminosity_distance is not None: |
There was a problem hiding this comment.
So no scaling will happen unless both total_mass and luminosity_distance are given?
| from gwtemplate.mass_time import MassTime | ||
| from gwtemplate import constants | ||
| import astropy.units as u | ||
| from astropy.constants import G, c ,M_sun |
There was a problem hiding this comment.
See my comment about m_sun_in_meters below.
| t_scaling = (self.params['total_mass'] / u.solMass * MTSUN_SI) | ||
| h_scaling = G * (self.params['total_mass']).to('kg') / (c**2 * self.params['distance'].to('m')) |
There was a problem hiding this comment.
Increasing my concern about units...
GWTemplate is the DDPC waveform generator for LISA. It functions through plugins built into the waveform models, which can then be called by the waveform generator to get the waveforms. I have added such a plugin to sxs, to allow sxs waveforms to easily be used for LISA science. Additionally, I moved some of the FFT-related routines to the time-series class. This is just to avoid having to Fourier transform all the modes when computing the frequency-domain polarizations in a particular direction.
📚 Documentation preview 📚: https://sxs--216.org.readthedocs.build/en/216/