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

add support for multi-returning-value functions in transform #4301

Merged
merged 16 commits into from
Feb 28, 2024

Conversation

WuShichao
Copy link
Contributor

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

@ahnitz
Copy link
Member

ahnitz commented Mar 28, 2023

@WuShichao It might be helpful if you give an example of what this is intended to enable.

@WuShichao
Copy link
Contributor Author

@ahnitz Good suggestion! Below is a transform from the geocentric frame to the SSB frame (based on #4289):

[waveform_transforms-tc_ssb+eclipticlongitude+eclipticlatitude+polarization_ssb]
name = custom
inputs = tc, ra, dec, polarization
tc_ssb, eclipticlongitude, eclipticlatitude, polarization_ssb = GEO_to_SSB(tc, ra, dec, polarization, useAstropy=True)

Copy link
Contributor

@cdcapano cdcapano left a 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.

@ahnitz
Copy link
Member

ahnitz commented Apr 3, 2023

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

@cdcapano
Copy link
Contributor

cdcapano commented Apr 3, 2023

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 CustomTransform. I'm just worried about breaking a corner case and/or significantly slowing down CustomTransform otherwise, since it's so widely used.

@WuShichao
Copy link
Contributor Author

WuShichao commented Apr 4, 2023

@cdcapano @ahnitz Thanks for these good suggestions! I will write several dedicated classes, for example, SSBtoGEO, GEOtoSSB, LISAtoGEO, GEOtoLISA, SSBtoLISA, and LISAtoSSB.

@WuShichao WuShichao requested a review from cdcapano April 4, 2023 22:20
@WuShichao
Copy link
Contributor Author

@cdcapano @ahnitz I have added a multi outputs version CustomTransformMultiOutputs, and 6 dedicated transform classes GEOToSSB, SSBToGEO, LISAToSSB, SSBToLISA, LISAToGEO, GEOToLISA. The build tests will have some issues, because this PR requires changes in #4289.

@ahnitz
Copy link
Member

ahnitz commented Apr 5, 2023

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

@WuShichao
Copy link
Contributor Author

@ahnitz Good suggestion! Let me move LISA stuff to #4289, so I guess @cdcapano may also have a look at that one.

@ahnitz
Copy link
Member

ahnitz commented Apr 20, 2023

@cdcapano Poke

@cdcapano
Copy link
Contributor

@WuShichao @ahnitz I just realized there was this PR sitting here from last year that I totally missed. Should this still be merged?

@WuShichao
Copy link
Contributor Author

@cdcapano Let me update this old branch to see if all the build tests have passed. Currently, this PR is not being used in my production code, but as @ahnitz said, " the ability for custom transforms though that can handle functions with multiple outputs" is helpful.

@WuShichao
Copy link
Contributor Author

@cdcapano @ahnitz All build tests are passed, is it good enough to be merged?

Copy link
Contributor

@cdcapano cdcapano left a 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

@cdcapano cdcapano merged commit 56b544f into gwastro:master Feb 28, 2024
32 of 33 checks passed
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
…#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
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
…#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
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
…#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants