Integration of pygrb aligntotalspin functionality into pycbc/transforms.py as a class #3756

SamuelH-97 · 2021-07-27T13:41:32Z

Proposing adding the aligntotalspin function from pygrb to the transforms.py code as a callable class. This does require the import of numpy and import of the jframe_to_l0frame function from pycbc.waveform.waveform_modes currently.

The class is able to rigidly rotate a binary so that the total angular momentum has the given inclination (iota) instead of the orbital angular momentum. It returns the new inclination, s1, and s2. s1 and s2 are dimensionless spin. The spins are assumed to be given in the frame defined by the orbital angular momentum.

The class has been written to fit with the style of other classes in the transforms.py code.

This is primarily used in the generation of injected binaries used to test the sensitivity of targeted GRB searches.

SamuelH-97 · 2021-07-27T13:55:15Z

I should add this is my first time doing a pull request or contributing here so please feel free to leave comments on if I did anything wrong with my request too

spxiwh · 2021-07-28T10:26:31Z

@SamuelH-97 Thanks for providing a first PR! It looks like you've set up everything correctly, and I've just approved the test code to run.

I'll ask @cdcapano if he would be willing to look over the content here, as this is something he knows much better than me. (@cdcapano if you can't do this, can you suggest someone else to look this over).

There are some style issues here that should be addressed first though. We do ask that everyone follows PEP-8 (for code) and the numpydoc (for documentation) styles. The automated code checker has picked up on a number of issues that should be addressed:

https://codeclimate.com/github/gwastro/pycbc/pull/3756

In most cases these should be trivial to fix, but in some cases (e.g. code duplication) it might make sense to keep what you have, but you should comment on that once you've resolved everything that you can.

I'll also leave a few specific comments on code style stuff in-line.

spxiwh · 2021-07-28T10:26:49Z

pycbc/transforms.py

@@ -612,7 +612,101 @@ def inverse_jacobian(self, maps):
        ref_mass=1.4
        mchirp = maps['mchirp']
        return (2.**(-1./5) * self.ref_mass / mchirp)**(5./6)
+
+######################################################################################################### TEST ################################################################################################################################################


Remove comments like this before this get merged.

spxiwh · 2021-07-28T10:27:22Z

pycbc/transforms.py

@@ -612,7 +612,101 @@ def inverse_jacobian(self, maps):
        ref_mass=1.4
        mchirp = maps['mchirp']
        return (2.**(-1./5) * self.ref_mass / mchirp)**(5./6)
+
+######################################################################################################### TEST ################################################################################################################################################
+import numpy as np


This code already imports numpy, don't need to do it again.

spxiwh · 2021-07-28T10:27:52Z

pycbc/transforms.py

+
+######################################################################################################### TEST ################################################################################################################################################
+import numpy as np
+from pycbc.waveform.waveform_modes import jframe_to_l0frame


Just to check (and not part of this PR). Have you checked that this code actually works? I'm not sure if it's been used for anything important ...

spxiwh · 2021-07-28T10:28:15Z

pycbc/transforms.py

+from pycbc.waveform.waveform_modes import jframe_to_l0frame
+
+class alignTotalSpin(BaseTransform):
+


Remove whitespace here

spxiwh · 2021-07-28T10:28:49Z

pycbc/transforms.py

+class alignTotalSpin(BaseTransform):
+
+    """Parameters
+    ----------


This isn't following the numpydoc formatting, and won't display correctly in the online docs. Can you update this, using other code as an example?

spxiwh · 2021-07-28T10:29:33Z

pycbc/transforms.py

+        self.news2 = news2
+        self._inputs = [self.iota, self.s1, self.s2, self.m1, self.m2, self.f_lower, self.phi_ref]
+        self._outputs = [self.newIota, self.news1, self.news2]
+        super(alignTotalSpin, self).__init__()   ############################################## what??????


Why the comment here? (While we are still using python2 we do need to support old-style super statements .... python2 will be retired soon though, so feel free to update once we do that!)

spxiwh · 2021-07-28T10:30:44Z

pycbc/transforms.py

+            l = np.array([0,0,1.])
+            theta1 = np.arccos(np.dot(maps[s1], l)/np.linalg.norm(maps[s1]))
+            theta2 = np.arccos(np.dot(maps[s2], l)/np.linalg.norm(maps[s2]))
+            phi12 = np.arccos(np.dot(maps[s1][:2], maps[s2][:2]) / np.linalg.norm(maps[s1][:2]) / \


In general multi-line statements like this are hard to parse. PEP-8 does have rules on this (and CodeClimate will pick up on this). I would probably align on the brackets, and the newline symbol is not needed.

spxiwh · 2021-07-28T10:31:02Z

pycbc/transforms.py

+            phi12 = np.arccos(np.dot(maps[s1][:2], maps[s2][:2]) / np.linalg.norm(maps[s1][:2]) / \
+                              np.linalg.norm(maps[s2][:2]))
+
+            #print(maps[s1])


Remove debugging statements before this gets onto the main branch

spxiwh · 2021-07-28T10:31:51Z

pycbc/transforms.py

+            news1 = np.zeros(3)
+            news2 = np.zeros(3)
+
+            output = \


There is a tool called black that you can use to align small blocks of code like this to be PEP-8 compliant .... I know this stuff sounds picky, but uniform code is much easier to parse in a long module like this one.

spxiwh · 2021-07-28T10:32:16Z

pycbc/transforms.py

+                      theta1, theta2,
+                      phi12)
+
+            newIota = output['inclination']      #not elegant, just quickly done for testing


If this is a "FIXME", then put it on the line above and use # FIXME (or fix it).

spxiwh · 2021-07-28T10:32:22Z

pycbc/transforms.py

+            news2[1] = output['spin2y']
+            news2[2] = output['spin2z']
+
+            #print(output['spin1x'])   ### WIP!!!!


spxiwh · 2021-07-28T10:32:28Z

pycbc/transforms.py

+            out = {self.newIota: iota, self.news1: s1, self.news2: s2}
+            return self.format_output(maps, out)
+
+######################################################################################################### TEST ################################################################################################################################################


Remove this as well

Hi, thank you for the feedback, and thanks for making me aware of issues I should have resolved before doing the PR. I'll take some time today to have a look over these and hopefully get this in a much better state.

cdcapano

This looks fine to me, although I agree with @spxiwh that you should address the PEP-8 and documentation issues (which looks like you're working on). My only additional request is you move the jframe_to_l0frame function to avoid circular imports; see my comment below.

cdcapano · 2021-07-30T13:37:44Z

pycbc/transforms.py

@@ -28,10 +28,12 @@
 from pycbc.waveform import parameters
 from pycbc.boundaries import Bounds
 from pycbc import VARARGS_DELIM
+from pycbc.waveform.waveform_modes import jframe_to_l0frame


This might cause circular imports. It's usually a bad idea to import functions from sub modules in top-level modules. I originally put this function in waveform_modes because that's where it was being used, but it would be useful at a higher level. Maybe move it to pycbc/pnutils.py? Then import from there (also make sure to add an import to pycbc/waveform/waveform_modes.py too).

Thanks for the help, I'll look into this asap

I would agree with Collin that if this function becomes useful outside of waveform_modes then it belongs somewhere else. Collin's moved this stuff about most recently, so his suggestion seems sensible.

SamuelH-97 · 2021-08-02T12:14:27Z

I used black as suggested by @spxiwh to resolve many of the PEP-8 issues. The only flags I'm seeing in the codeclimate page now are for similar lines of code (which I think need to be that way), and lines that are too long. Are these still things you'd suggest I need to fix? Should I just break the long lines?

spxiwh · 2021-08-02T12:24:46Z

@SamuelH-97 Thanks for this. In some cases here I think black should have been able to keep the 79 character line limit. I think you probably ran it without specifying this. I quote from the black documentation:

If you’re paid by the line of code you write, you can pass --line-length with a lower number. Black will try to respect that. However, sometimes it won’t be able to without breaking other rules. In those rare cases, auto-formatted code will exceed your allotted limit.

In some cases here it probably will keep > 79 character lines if you use line-length, but please do try running black with line-length 79 first. Then I think this is good to go.

I agree that the code repetition point here is not a problem. It would add confusion and reduce clarity to make this a function called twice.

pycbc/transforms.py

a-r-williamson · 2021-08-03T15:39:48Z

pycbc/transforms.py

-
+
+    name = "align_total_spin"
+
    def __init__(self, iota, s1, s2, m1, m2, f_lower, phi_ref, newIota, news1, news2):


I think we should be passing in & out the x/y/z components of the spin, rather than packaging them into vectors.

a-r-williamson · 2021-08-05T12:27:40Z

The changes up until now have been tested and work if the suggested bugfix in #3760 is included.

a-r-williamson · 2021-10-12T16:33:09Z

This is the first significant PR aiming to address the issue #3468

SamuelH-97 · 2021-10-13T16:00:19Z

Hi, sorry for the extended break on this request. I've moved the jframe_to_i0frame to pnutils.py as suggested. I assume I should probably do the same with it's sibling i0frame_to_jframe.

a-r-williamson · 2021-10-26T13:10:39Z

Hi, sorry for the extended break on this request. I've moved the jframe_to_i0frame to pnutils.py as suggested. I assume I should probably do the same with it's sibling i0frame_to_jframe.

I think that's sensible. @cdcapano would you be happy with these functions being moved? They seem to be unused anywhere else.

SamuelH-97 · 2021-11-01T15:19:38Z

With these changes and fixes now pushed, would it be possible for you to take another look @cdcapano @spxiwh and make sure you're happy with them?

ahnitz · 2021-11-29T14:01:38Z

@a-r-williamson @SamuelH-97 Can you rebase so we can verify the unittests / CC pass?

ahnitz · 2021-11-29T15:44:54Z

@a-r-williamson @SamuelH-97 Are you sure this branch is as you've tested? The unittests are all failing for a sensible reason. The _formatdocstr function was private to the module you move the function from and hasn't been imported over.

SamuelH-97 · 2021-11-29T16:03:20Z

@a-r-williamson @SamuelH-97 Can you rebase so we can verify the unittests / CC pass?

Rebase is done

SamuelH-97 · 2021-11-29T16:03:33Z

@a-r-williamson @SamuelH-97 Are you sure this branch is as you've tested? The unittests are all failing for a sensible reason. The _formatdocstr function was private to the module you move the function from and hasn't been imported over.

Will get that fixed asap

a-r-williamson · 2021-12-01T16:10:13Z

pycbc/pnutils.py

+    {mass1}
+    {mass2}
+    {f_ref}


These and similar parts of the docstrings (parameters inside {} in inputs and outputs) will need to be written out explicitly as they're not going to be parsed like they would be in waveform_modes.py. For example:

Suggested change

{mass1}

{mass2}

{f_ref}

mass1 : float

First component mass in solar mass units

mass2 : float

Second component mass in solar mass units

f_ref: float

Reference frequency

titodalcanton · 2021-12-01T16:13:33Z

Please rebase on master again, as we recently did some documentation fixes that may affect this. Sorry!

Co-authored-by: Andrew R. Williamson <[email protected]>

…. May need to do the same for i0frame_to_jframe

…code checks

…checks

…ns to fit expected formatting

…I0frme and it's sibling function

a-r-williamson · 2021-12-16T17:22:21Z

The previous check failures are now resolved.

a-r-williamson · 2021-12-17T14:43:11Z

@ahnitz Given the current state of this, can it be merged? It's been tested and the builds are passing.

Collin not responding

spxiwh reviewed Jul 28, 2021

View reviewed changes

spxiwh requested changes Jul 28, 2021

View reviewed changes

spxiwh assigned cdcapano Jul 28, 2021

spxiwh requested a review from cdcapano July 28, 2021 10:32

cdcapano previously requested changes Jul 30, 2021

View reviewed changes

a-r-williamson reviewed Aug 3, 2021

View reviewed changes

a-r-williamson mentioned this pull request Aug 9, 2021

Infinite loop in ordering of waveform transforms #3762

Closed

a-r-williamson added this to the PyGRB - Phase 1 milestone Oct 12, 2021

a-r-williamson added the PyGRB PyGRB development label Oct 12, 2021

a-r-williamson requested review from spxiwh and cdcapano October 26, 2021 13:20

a-r-williamson assigned SamuelH-97 Nov 9, 2021

SamuelH-97 force-pushed the master branch from 865416e to 467e1dc Compare November 29, 2021 15:49

a-r-williamson reviewed Dec 1, 2021

View reviewed changes

SamuelH-97 and others added 21 commits December 16, 2021 07:51

Update pycbc/transforms.py

e22633c

Co-authored-by: Andrew R. Williamson <[email protected]>

Update pycbc/transforms.py

d2d65d9

Co-authored-by: Andrew R. Williamson <[email protected]>

Fixing bugs I introduced in AlignTotalSpin.

1613752

Correcting output when no-/aligned-spins passed

e75c6a6

Following standard input/output param handling

aa6f6c2

Fix typo

e509731

Adding the transform to dict for config use

a7d83df

Make frame transform work on sets of inputs

85fb9b1

Moved jframe_to_l0frame function to pnutils.py from waveform_modes.py…

2c3e49a

…. May need to do the same for i0frame_to_jframe

Moved l0frame_to_jframe function from waveform_modes to pnutils

fbbc69c

made alterations to ensure my added and moved code is compliant with …

fef5fc6

…code checks

made alterations to ensure transforms.py to make compliant with code …

eafa7a5

…checks

made alterations to ensure transforms.py is compliant with code checks

a167b11

made alterations to ensure transforms.py is compliant with code checks

a7f26e6

made alterations to ensure transforms.py is compliant with code checks

ea2958b

made alterations to ensure transforms.py is compliant with code checks

fceddb9

attempting fix for like 2299 of transforms.py

228ea8c

Removed _formatdocstr lines in pnutils

b0e6ed6

Updated docstrings in l0frame_to_jframe and jframe_to_i0frame functio…

bf751a5

…ns to fit expected formatting

added data type labels to docstrings

ed643b7

added blank line under dictionary header in docstrings for jframe_to_…

48f7d6c

…I0frme and it's sibling function

a-r-williamson force-pushed the master branch from af82979 to 48f7d6c Compare December 16, 2021 15:54

spxiwh approved these changes Dec 17, 2021

View reviewed changes

spxiwh removed the request for review from cdcapano December 17, 2021 16:15

a-r-williamson merged commit 0bb2754 into gwastro:master Dec 17, 2021

		from pycbc.waveform.waveform_modes import jframe_to_l0frame

		class alignTotalSpin(BaseTransform):



		name = "align_total_spin"

		def __init__(self, iota, s1, s2, m1, m2, f_lower, phi_ref, newIota, news1, news2):

-    {mass1}
-    {mass2}
-    {f_ref}
+    mass1 : float
+        First component mass in solar mass units
+    mass2 : float
+        Second component mass in solar mass units
+    f_ref: float
+        Reference frequency

Integration of pygrb aligntotalspin functionality into pycbc/transforms.py as a class #3756

Integration of pygrb aligntotalspin functionality into pycbc/transforms.py as a class #3756

Conversation

SamuelH-97 commented Jul 27, 2021

SamuelH-97 commented Jul 27, 2021

spxiwh commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdcapano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamuelH-97 commented Aug 2, 2021

spxiwh commented Aug 2, 2021

Choose a reason for hiding this comment

a-r-williamson commented Aug 5, 2021

a-r-williamson commented Oct 12, 2021 • edited Loading

SamuelH-97 commented Oct 13, 2021

a-r-williamson commented Oct 26, 2021 • edited Loading

SamuelH-97 commented Nov 1, 2021

ahnitz commented Nov 29, 2021

ahnitz commented Nov 29, 2021

SamuelH-97 commented Nov 29, 2021

SamuelH-97 commented Nov 29, 2021

Choose a reason for hiding this comment

titodalcanton commented Dec 1, 2021

a-r-williamson commented Dec 16, 2021

a-r-williamson commented Dec 17, 2021

a-r-williamson commented Oct 12, 2021 •

edited

Loading

a-r-williamson commented Oct 26, 2021 •

edited

Loading