FEAT: Subclass of GWSignalWaveformGenerator for eccentric waveforms #1034
Conversation
adivijaykumar
changed the title
adivijaykumar
requested review from
ColmTalbot,
asb5468,
Copilot and
mj-will
and removed request for
asb5468
There was a problem hiding this comment.
Pull request overview
This PR adds a convenience subclass EccentricGWSignalWaveformGenerator that simplifies the creation of eccentric waveform generators by automatically setting eccentric=True.
Changes:
- Added new
EccentricGWSignalWaveformGeneratorclass that inherits fromGWSignalWaveformGeneratorand setseccentric=Trueby default
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class EccentricGWSignalWaveformGenerator(GWSignalWaveformGenerator): | ||
| """ | ||
| Subclass that initializes an eccentric GWSignal Waveform Generator. | ||
| Equivalent to `GWSignalWaveformGenerator` with `eccentric=True`. | ||
| See documentation of `GWSignalWaveformGenerator` for more information. | ||
| """ | ||
|
|
||
| def __init__(self, **kwargs): | ||
| super().__init__(eccentric=True, **kwargs) |
There was a problem hiding this comment.
Consider adding test coverage for the new EccentricGWSignalWaveformGenerator class. While the implementation is straightforward, tests would ensure that the class properly initializes with eccentric=True and that the eccentric parameters are correctly included in the parameters (not in defaults). A simple test could verify that instantiating this class is equivalent to GWSignalWaveformGenerator(eccentric=True, ...).
ColmTalbot
left a comment
ColmTalbot
left a comment
There was a problem hiding this comment.
I'm happy to add this as there seems to be a desire for it, although I don't really see how it is preferable to passing an argument.
| """ | ||
| Subclass that initializes an eccentric GWSignal Waveform Generator. | ||
| Equivalent to `GWSignalWaveformGenerator` with `eccentric=True`. | ||
| See documentation of `GWSignalWaveformGenerator` for more information. |
There was a problem hiding this comment.
I think it's worth specifying that this doesn't do any kind of checking that the requested model actually supports eccentricity.
|
Also adding @IsobelMarguarethe and @lorenzopompili00 to the discussion. |
@ColmTalbot it reduces the number of extra arguments that have to be included in a bilby_pipe ini file. An alternative that we discussed on Monday could be moving the eccentric=True argument to the waveform_arguments, to avoid having to pass a separate dict of arguments to the waveform generator. |
adivijaykumar
force-pushed
the
eccentric-subclass
branch
from
5f3273a to
b3a3075
Compare
3 participants
Adds a subclass of GWSignalWaveformGenerator for eccentric waveforms.
I realize that avoiding subclassing/writing source models was one of the reasons we moved away from the old way of calling waveforms. I see these subclasses as "convenience functions", and since it doesn't duplicate a lot of code I am in favour of adding them. But I can also see arguments to the contrary.