add rational fits #223

md-arif-shaikh · 2024-09-25T09:38:11Z

Add rational_fit as an alternative method to interpolate the omega at the extrema.
This PR introduces a new option in extra_kwargs for omega_gw_extrema_interpolation_method which defaults to the spline-based interpolation.

omega_gw_extrema_interpolation_method can be set to either of the following values:

spline: Uses InterpolatedUnivariateSpline with k=3 (as default).
rational_fit: Uses rational approximation as implemented in polyrat.StabilizedSKRationalApproximation

rational_fit improves the fit to the omega_gw at pericenter and apocenters, particularly near the merger where spline struggles to remain monotonic.

md-arif-shaikh · 2024-10-27T06:17:10Z

This PR is quite long, so I want to highlight the main changes

utils.py: general rational fit functions added here
eccDefinition.py: rational fit functions for fit to the omega_gw_extrema added. This is the most important function to be reviewed.

Other changes including the tests are straightforward. I Had to change a few things to make the tests pass because we have changed the default to rational_fit. More changes are to be made to the tests but that can be done later in a separate PR.

md-arif-shaikh · 2024-10-27T06:21:30Z

Hi @vijayvarma392, this PR is now ready to be reviewed. I have summarized the main changes here. I think at this point, I would like to have your comments for making further changes.

vijayvarma392 · 2024-10-30T20:46:37Z

gw_eccentricity/eccDefinition.py

+                Dictionary of arguments to be passed to the rational
+                fit function. Defaults are set using
+                `utils.get_default_rational_fit_kwargs`
+


You should indicate here already that only one of spline_kwargs or rational_fit_kwargs are allowed.
Question: Would it be better to have interp_kwargs, which has a key 'interp_type' that can be 'spline' or 'rational' fit? In the future, if we add extra options like Toni's PN inspired fits, that would make it easier.
This is more of a question for you rather than a suggestion because I don't know if that will make the code more complicated than it needs to be. Either way, needs to be clear that only one is allowed.

We use spline kwargs for things other than interpolating the omega_gw extrema. For example, we interpolate zeroecc data using spline. So providing just rational_fit kwargs won't work because rational_fit is only good for interpolating monotonic trends.

Basically we will need to make all interpolation thing work with a single method if we want the user to provide only one of them. Here, we wanted to use rational fit for the omega envelope mainly.

What do you suggest?

vijayvarma392 · 2024-10-30T20:48:18Z

gw_eccentricity/eccDefinition.py

+                        using models like SEOB or TEOB.
+                    - Faster to construct and evaluate.
+                    - Since it fits through every data point, it may exhibit oscillatory 
+                        behavior, particularly near the merger.


near the merger, especially for noisy NR data.

vijayvarma392 · 2024-10-30T20:49:08Z

gw_eccentricity/eccDefinition.py

+                        modes from numerical simulations.
+                    - Better monotonic behaviour, particularly near the merger.
+                    - Significantly slower compared to the `spline` method. This is because
+                        finding optimal numerator and denominator degree needs several iterations


Can you say a rough number instead of "Significantly slower"?

vijayvarma392 · 2024-10-30T20:50:58Z

gw_eccentricity/eccDefinition.py

+        self.rational_fit_kwargs["verbose"] = self.debug_level
+        # keep history of rational fit degree and nonmonotonicity of the corresponding fits
+        self.rational_fit_nonmonotonicity_history = {"pericenters": {}, "apocenters": {}}
+        # Update the degrees used to construct the final fits


Should the comment be: This is used to keep track and update the degrees used to construct the final fits?

Okay, so I am using rational_fit_nonmonotonicity_history to keep track of what degrees are being iterated over and the corresponding behaviour, i.e., is it nonmonotonic? And using rational_fit_degrees to store the final degree used to build the fits. Instead of using "update the degrees", I changed it to "store degrees" to better convey the purpose.

vijayvarma392 · 2024-10-30T20:51:20Z

gw_eccentricity/eccDefinition.py

        self.available_averaging_methods \
            = self.get_available_omega_gw_averaging_methods()
        self.debug_level = self.extra_kwargs["debug_level"]
+        # set verbose to debug_level. If verbose is True, then it prints information
+        # of each iteration for rational fits for a given degree.
+        self.rational_fit_kwargs["verbose"] = self.debug_level


Why not just call it debug_level?

"verbose" is the keyword used in the routine for rational fit, so I have kept that the same and instead assign value to it using debug_level

vijayvarma392 · 2024-10-30T20:53:42Z

gw_eccentricity/eccDefinition.py

@@ -1184,6 +1228,180 @@ def interp_extrema(self, extrema_type="pericenters"):
            raise Exception(
                f"Sufficient number of {extrema_type} are not found."
                " Can not create an interpolant.")
+
+    def get_rat_fit(self, x, y):


hmm, don't like the name...come up with a way to not call the two things get_rat_fit and get_rational_fit? Easily confused.

changed get_rat_fit to get_rational_fit_wrapper

vijayvarma392 · 2024-10-31T19:26:28Z

gw_eccentricity/eccDefinition.py

+        This function ensures that the rational fit uses the optimal degree for
+        the numerator and denominator. It first checks for nonmonotonicity in the
+        fit's derivative and lowers the degrees if necessary. Only if the initial
+        degree was monotonic (no reduction needed), it attempts to increase the


"Only if the initial degree was monotonic" --> "Only if the fit for the initial degree was monotonic"

vijayvarma392 · 2024-10-31T19:28:03Z

gw_eccentricity/eccDefinition.py

+        # make sure that description is not None. A description is needed to
+        # update the optimal values of the numerator and denominator degrees
+        # used to build the final rational fit
+        if description is None:


description --> data_name?

vijayvarma392 · 2024-10-31T19:29:01Z

gw_eccentricity/eccDefinition.py

+        fit's derivative and lowers the degrees if necessary. Only if the initial
+        degree was monotonic (no reduction needed), it attempts to increase the
+        degrees to find a higher-degree fit. If increasing the degrees causes
+        nonmonotonicity, it reverts to the previous valid monotonic fit.


Say something about how the initial degrees should be set? Through self.rational_fit_kwargs?

vijayvarma392 · 2024-10-31T19:30:50Z

gw_eccentricity/eccDefinition.py

+        # value is found by doing some iterations.
+        if not (self.rational_fit_kwargs["num_degree"] and self.rational_fit_kwargs["denom_degree"]):
+            self.rational_fit_kwargs["num_degree"], self.rational_fit_kwargs["denom_degree"] \
+                = self.get_approximate_degree_for_rational_fit()


Does this still work if the user gives only one of num_degree and denom_degree?

No, I have changed this part of the code so that it does the following: If both are None, i.e., unspecified only then use the function to set the approximate degree to both num and denom. If any one of the degrees is given, use it to set the other.

vijayvarma392 · 2024-10-31T19:35:44Z

gw_eccentricity/eccDefinition.py

+            num_degree, denom_degree = 6, 6
+        else:
+            num_degree, denom_degree = 5 + int(np.log10(approximate_num_orbits)), 5 + int(np.log10(approximate_num_orbits))
+        return num_degree, denom_degree


If you check for approximate_num_orbits <= 5 once, you don't need to check for approximate_num_orbits > 5 again in elif. Something like:

if orbits <= 5: bla elif orbits <= 20:

How did you arrive at these choices? Add some comments about how you got there and how important these particular choices are.

corrected. These are based on limited investigations. It needs a lot more tests to set more optimal values which I intend to do once this is merged. I have noted this using TODO at the top

vijayvarma392 · 2024-10-31T19:37:34Z

gw_eccentricity/eccDefinition.py

+                = self.get_approximate_degree_for_rational_fit()
+
+        rat_fit = self.get_rat_fit(x, y)
+        t = np.arange(x[0], x[-1], self.t[1] - self.t[0])


best not to call this t. There is already a self.t, and the natural thing to do is to assume they are the same.

vijayvarma392 · 2024-10-31T19:40:38Z

gw_eccentricity/eccDefinition.py

+        degrees_were_reduced = False
+
+        # Check for nonmonotonicity and lower degrees if needed
+        while self.check_if_domega_dt_is_nonmonotonic(t, rat_fit(t), 1.0, description):


Should the function be called something other than check_if_domega_dt_is_nonmonotonic? Isn't the data allowed to be things other than omega?

Ok, I have changed it to check_if_first_derivative_is_not_strictly_monotonic

vijayvarma392 · 2024-10-31T19:41:53Z

gw_eccentricity/eccDefinition.py

+                          debug_level=self.debug_level, important=False)
+
+            rat_fit = self.get_rat_fit(x, y)
+        # If no degrees were reduced, try increasing the degree for better fit


for a better fit

vijayvarma392 · 2024-10-31T19:44:36Z

gw_eccentricity/eccDefinition.py

+                self.rational_fit_kwargs["num_degree"], self.rational_fit_kwargs["denom_degree"] \
+                    = new_num_degree, new_denom_degree
+                new_rat_fit = self.get_rat_fit(x, y)
+                if self.check_if_domega_dt_is_nonmonotonic(t, new_rat_fit(t), 1.0, description):


Looks like this check happens twice for the same fit in some cases? Maybe rework the code so that that doesn't happen?

vijayvarma392 · 2024-10-31T19:48:19Z

gw_eccentricity/eccDefinition.py

@@ -1597,6 +1814,33 @@ def measure_ecc(self, tref_in=None, fref_in=None):
        # return measured eccentricity, mean anomaly and reference time or
        # frequency where these are measured.
        return self.make_return_dict_for_eccentricity_and_mean_anomaly()
+
+    def build_omega_gw_extrema_interpolants(self):


Needs a docstring explaining what it's doing and why.

add rational fits

07cd500

md-arif-shaikh marked this pull request as draft September 25, 2024 09:38

md-arif-shaikh added 27 commits September 26, 2024 08:51

add documentation and restructure rational_fit

107fbf2

add omega_gw_extrema_interpolation_method

7b527fd

use approximate orbits to guess degree for rational fit

59eb2be

use optimal degree function only user provided degree is None

37ec2b3

improve debug message

546b406

more polishing

89fc181

improve omega interpolant documentation

b17483c

minor

92ddab0

copy documentation to gw_eccentricity

30f7376

minor

bf5619c

Merge branch 'main' into rational_fit

f01f538

raise exception if degree=1 is attempted to be lowered

326b10a

Merge branch 'main' into rational_fit

0805bb0

fix nonmonotic egw by increasing rational fit degree

42a6755

add informative message

4a5e4c9

turn on debug

95e60e1

refactor egw checking and fixing with rational fits

b4b1ab3

fix typos

5a05690

add variable to store final fit degrees

83b7d08

remove fallback option

0fca25f

improve documentation, change to rational_fit

065776e

make tests pass

671c54a

change egw monotonicity check to previous version

ae873cf

add docs under get_rational_fit

336227d

update regression data and test based on interp method

c3b4eb8

fix regression test

9c376db

ignore deprecationwarning from polyrat

03c75ae

test units for spline for now

c2c361f

md-arif-shaikh marked this pull request as ready for review October 27, 2024 06:18

md-arif-shaikh requested a review from vijayvarma392 October 27, 2024 06:18

vijayvarma392 requested changes Oct 30, 2024

View reviewed changes

vijayvarma392 requested changes Oct 31, 2024

View reviewed changes

md-arif-shaikh added 9 commits November 5, 2024 19:51

address comments

07336b0

update -> store

6f2d3ae

Merge branch 'main' into rational_fit

9048299

separate interpolation for extrema and non extrema data

0e89ed4

copy over to gw_eccentricity

d41645e

add checks for only one key in extrema_interp_kwargs

ef8aaa3

do only extra_kwargs not None

d158f31

Merge branch 'main' into rational_fit

b637871

rename interp kwargs

c916d70

add rational fits #223

Are you sure you want to change the base?

add rational fits #223

Conversation

md-arif-shaikh commented Sep 25, 2024 • edited Loading

md-arif-shaikh commented Oct 27, 2024

md-arif-shaikh commented Oct 27, 2024

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

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

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

md-arif-shaikh commented Sep 25, 2024 •

edited

Loading