-
Notifications
You must be signed in to change notification settings - Fork 356
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
add support for multi-returning-value functions in transform #4301
Conversation
@WuShichao It might be helpful if you give an example of what this is intended to enable. |
@ahnitz Good suggestion! Below is a transform from the geocentric frame to the SSB frame (based on #4289):
|
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'm not sure about this. First, there's a spot where it looks like you could break some functionality (changing a set to a list). It's also changes what the CustomTransform
is meant for. It's really only meant for simple transforms that are many-to-one.
I think for the sort of thing you're looking to do, it would be better to write a transform class, rather than trying to use the CustomTransform
. There are several transform classes defined in transforms.py
that do what you're looking to do here: take in one or more inputs and produce multiple outputs. See the MchirpQToMass1Mass2 transform for an example.
So, rather than modify CustomTransform
write an GEOToSSB
transform class that applies all the transforms you're doing here. This should prove to be quite useful in the future.
@cdcapano @WuShichao A dedicated transform might make sense here. @WuShichao Can you take a look at that? @cdcapano I think we may still want the ability for custom transforms though that can handle functions with multiple outputs. Is there a reason we should restrict to only one? Maybe in a separate 'Custom Class'? |
A custom transform that can handle multiple outputs could be useful. But yeah, I think it would be better to do that as a separate class, rather than try to squeeze it into |
@WuShichao I'd just have the additional custom multi valued transform in this PR. That way it doesn't dependon your other PR and can be merged first. |
@cdcapano Poke |
@WuShichao @ahnitz I just realized there was this PR sitting here from last year that I totally missed. Should this still be merged? |
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.
Looks good. Sorry for not reviewing it earlier
…#4301) * add support for multi-value functions in transform * fix cc issue * Update transforms.py * fix cc issue * fix * Update transforms.py * Update transforms.py * fix cc issue * Update transforms.py * add 7 classes * fix cc issues * move LISA stuff to another PR * Update transforms.py
…#4301) * add support for multi-value functions in transform * fix cc issue * Update transforms.py * fix cc issue * fix * Update transforms.py * Update transforms.py * fix cc issue * Update transforms.py * add 7 classes * fix cc issues * move LISA stuff to another PR * Update transforms.py
…#4301) * add support for multi-value functions in transform * fix cc issue * Update transforms.py * fix cc issue * fix * Update transforms.py * Update transforms.py * fix cc issue * Update transforms.py * add 7 classes * fix cc issues * move LISA stuff to another PR * Update transforms.py
@ahnitz This PR is adding support for multi-returning-value functions in the
waveform_transforms
section in config file. PR #4289 will use this. As Alex suggested, @cdcapano, can you be the reviewer for this PR if you have time?