Updated piecewise orbital model #1545

poneill129 · 2023-03-31T15:19:52Z

Tried merging these changes into the existing PR but looks like its opened as a new one, apologies!
Tagged reviewers and commenters so you don't miss it. (@scottransom @aarchiba @dlakaplan)

Had a chance to address Anne's comments on the piecewise orbital model.

I will add the responses to the comments in the original PR #1378 as well as here in a follow-up comment.

codecov · 2023-03-31T16:58:03Z

Codecov Report

Patch coverage: 67.96% and no project coverage change.

Comparison is base (25f38ca) 68.27% compared to head (484508e) 68.28%.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #1545    +/-   ##
========================================
  Coverage   68.27%   68.28%            
========================================
  Files         100      101     +1     
  Lines       23111    23596   +485     
  Branches     4010     4160   +150     
========================================
+ Hits        15779    16112   +333     
- Misses       6342     6427    +85     
- Partials      990     1057    +67

Files Changed	Coverage Δ
src/pint/models/binary_bt.py	`66.79% <64.75%> (-21.21%)`	⬇️
...nt/models/stand_alone_psr_binaries/BT_piecewise.py	`71.07% <71.07%> (ø)`
src/pint/models/__init__.py	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dlakaplan · 2023-04-06T16:25:29Z

Can you share in the docstring and/or here how such a model is defined?

scottransom · 2023-06-14T15:08:58Z

This looks great, @poneill129. Once the docstring is added to as requested by David K, and the minor conflict in src/pint/models/__init__.py is fixed, do you think this is basically ready to merge? My student is wanting to test/use it and it would be better to do that (I think) if it is merged than trying to work in a side branch.

poneill129 · 2023-06-14T15:37:25Z

This looks great, @poneill129. Once the docstring is added to as requested by David K, and the minor conflict in src/pint/models/__init__.py is fixed, do you think this is basically ready to merge? My student is wanting to test/use it and it would be better to do that (I think) if it is merged than trying to work in a side branch.

Hi, the docstring turned into a more involved problem than anticipated. Trying to replicate the convention for other standalone binary models I found myself in a rabbit hole.

As part of the model declaration an arbitrary piece and associated range are added. Then attempting to add a piece directly to the model (not via the pint facing binary_piecewise module) is bugged in that the dict that is used in the update_binary_object() causes an error when adding a piece.

I will need to find a way of producing the bug to report it, I will emphasise that this does not happen when piecwise model is declared via the wrapper module. Effectively it’s a timing model that I can only make work via the pint interface.

scottransom · 2023-09-05T13:39:35Z

@poneill129 Coming back to this again... Is the current status still accurate based on your last comment above with an error when you try to add a piece? If so, might you be able to email me with some example code as to how/when that is happening? And also, what is your "wrapper" module that allows things to work? Is there an example usage of that? I'm going to start playing with this this week as two of my students really do need it starting now-ish.

poneill129 · 2023-09-07T10:39:02Z

Hi @scottransom, yes I think I've cracked it! I'll email you a demo script asap and stage my changes.

Just to keep it clear, when I refer to a stand alone model I'm talking about BT_model.py/BT_piecewise.py, and when I say timing model I mean "obtained from get_model()".

So my issue was in documenting the piecewise stand alone model in the style of the BT model, I tried following the steps albeit with the piecewise stand alone model, not realising update_object(t,updates) is not how the arguments are passed by the timing model (anymore?). I needed to step away at that point because I could not see how the binary_instance was "noticing" the toas while the piecewise variables worked fine during a "get_model" or residual calculation. Can think of this as the stand alone model is in a state of waiting for toas to allocate to its predefined pieces.

I thought this was an indication that there was some foundational oversight that meant that I couldn't operate on it as a stand alone model. But looking over it with fresh eyes I've found that this was not the case, I did catch a minor bug in the toa-piece allocation step (never was relevant when calling using a timing model but popped up during this exploration). Just folding this into the documentation now, along with the recommended changes to "XR_0000" to denote piece ranges. I suppose if others agree that the steps included in the documentation for BT_model.py are for an old update_input method there's something else that can be changed.

src/pint/models/binary_piecewise.py

dlakaplan · 2023-09-07T13:59:07Z

src/pint/models/binary_piecewise.py

+        self.remove_param("SINI")
+        self.T0.value = 1
+        self.A1.value = 1
+        self.add_group_range(50000, 51000, frozen=False, j=0)


Can this be initialized with None instead of random MJDs?

src/pint/models/binary_piecewise.py

dlakaplan · 2023-09-07T14:04:17Z

Hi, @poneill129 . Great that you made progress. I started to look at your code but I realize you may have some updates waiting to go. Should I keep reviewing things now or wait?

poneill129 · 2023-09-07T14:15:58Z

Hi @dlakaplan, The proposed change is only very small, affecting only the "toa_belongs_in_group" function in BT_piecewise.py. I'm still trying to pull things together. Thank you for the comments you've already given, if you want to keep going (avoiding toa_belongs_in_groups) I'll pull the changes in while I go!

dlakaplan · 2023-09-07T14:49:00Z

src/pint/models/__init__.py

@@ -21,6 +21,7 @@
 # Import all standard model components here
 from pint.models.astrometry import AstrometryEcliptic, AstrometryEquatorial
 from pint.models.binary_bt import BinaryBT
+from pint.models.binary_piecewise import BinaryBTPiecewise


This is fine, but you could also consider defining BinaryBTPiecewise in binary_bt.py (like how dispersion_model.py has DispersionDM and DispersionDMX

(the standalone model piece should be in a separate file)

I've shifted the binary_piecewise class into BTmodel now, made changes in init to accommodate

src/pint/models/binary_piecewise.py

dlakaplan · 2023-09-07T15:08:40Z

src/pint/models/binary_piecewise.py

+                )
+            )
+
+    def lock_groups(self):


Is this needed? Is it used?

So I found myself needing this function as validates/setups cannot be run before all pieces and corresponding group boundaries are declared. Otherwise there are floating piecewise parameters that don't have a group to go with them (which validate does not like). The function is intended to be be used after all add_group_range and add_piecewise_param have been called

dlakaplan · 2023-09-07T15:10:54Z

src/pint/models/binary_piecewise.py

+        for t0n in T0X_mapping.values():
+            T0Xs[t0n] = getattr(self, t0n).quantity
+
+        for t0_name, t0_value in T0Xs.items():


would it make more sense to do a single loop over the PLB/PUB mappings? And then for each index check whether there was a A1X or T1X value and set them if so?

Implemented the sorting algorithm described

dlakaplan · 2023-09-07T15:12:10Z

src/pint/models/binary_piecewise.py

+
+            if j_plb != j_pub:
+                raise ValueError(
+                    f"Group boundary mismatch error. Number of detected upper bounds: {j_plb}. Number of detected upper bounds:{j_pub}"


probably include a space before the values

Added extra space

dlakaplan · 2023-09-07T15:15:37Z

You might look at toa_select.py. That has tools to do some of what you are talking about. You can see how it's used by DMX (for instance).

dlakaplan · 2023-09-07T15:19:35Z

src/pint/models/binary_piecewise.py

+           str elements, look like ['0000','0001'] for two TOAs where one refences T0X_0000 or T0X_0001.
+        """
+        # asks the model what group a single TOA/list of TOAs
+        barycentric_toa = self._parent.get_barycentric_toas(toa)


I think passing a TOAs object here would be fine. then you don't have to worry about re-barycentering (which can be slow). Again, look at how dmx_dm does things.

reduced to barycentric_toa = toa (this is one of a few convenience functions which are going to change with testing)

dlakaplan · 2023-09-07T15:20:58Z

src/pint/models/binary_piecewise.py

+            )
+        return group_indexes
+
+    def get_group_indexes_in_four_digit_format(self):


Is this needed? If you have the method above to return integers can't you just use:

[f"{int(...):04d" for x in ...]

Yeah, this was an one of the functions I would use in the testing script. Does nothing essential, scrapped

dlakaplan · 2023-09-07T15:22:53Z

src/pint/models/binary_piecewise.py

+            )
+        return group_indexes
+
+    def get_T0Xs_associated_with_toas(self, toas):


maybe instead of having specialized methods for TOX and A1X, you can have 1 method to see which group(s) TOAs are associated with? Then the TOX and A1X can just be gotten using the existing mappings.

implemented new function paramX_per_toa('T0',toa) which does as was described returns same as get_T0Xs_associated_with_toas. This function changes the structure of the testing suite since now a lot of convenience functions can be consolidated into this

dlakaplan · 2023-09-07T15:23:18Z

src/pint/models/stand_alone_psr_binaries/BT_piecewise.py

+from .binary_generic import PSR_BINARY
+
+
+def extract(lst):


Is this needed?

Nope, and its gone now

dlakaplan · 2023-09-07T15:24:14Z

src/pint/models/binary_piecewise.py

+        """Query whether a TOA/list of TOAs belong(s) to a specific group
+        Parameters
+        ----------
+        toas :


try to be explicit with the types/class references. So this would be of type pint.toa.TOAs.

I left some of these unchanged in the interest of time, since some of these functions are being removed

dlakaplan · 2023-09-07T15:26:29Z

Hi, @poneill129 . Overall this looks good, although I think you could simplify some of the smaller functions. But I notice that you don't have any tests yet. It would be great to have some tests like:

make a model with upper/lower ranges for the whole data-set, and compare the binary delays with existing BT
make a models divided into two ranges. make 2 BT models and compare appropriately
test the various setup/adding/removing functions. Make sure that errors are raised appropriately for the conditions you give (like overlapping ranges)
You might look at some of the tests for DMX, WaveX or similar to see various examples.

poneill129 · 2023-09-07T15:45:32Z

Hi, @poneill129 . Overall this looks good, although I think you could simplify some of the smaller functions.

Cheers again for these, and I'm working my way through them now

But I notice that you don't have any tests yet. It would be great to have some tests like:

I don't know if it's been missed or I'm misunderstanding but I should have a test file in the form of "test_BT_piecewise.py". It contains tests that might be similar to:

make a models divided into two ranges. make 2 BT models and compare appropriately

make a model with upper/lower ranges for the whole data-set, and compare the binary delays with existing BT

I'm fairly sure there's a test in there for testing the error raised when overlapping groups are declared

test the various setup/adding/removing functions. Make sure that errors are raised appropriately for the conditions you give (like overlapping ranges)

Regardless, I'm going to look into the test file again and identify which tests closely resemble what you've outlined, and I'll adapt what is there if I can't find an equivalent test.

You might look at some of the tests for DMX, WaveX or similar to see various examples.

Will do, thanks for the pointer!
Cheers for these again!

dlakaplan · 2023-09-07T15:50:36Z

Great - I missed the tests since the browser didn't show me the code initially.

poneill129 · 2023-09-08T10:51:35Z

I've folded in a lot of the changes discussed, though in doing so, some of the functions in what was binary_piecewise.py have been made redundant. Those functions were used in the test suite which I am now working on fixing.

poneill129 · 2023-09-08T11:14:34Z

This PR is a little untested. This should be functional, and I'm trialing some demo notebooks which I will circulate for you to play around with while I address the testing suite.

Currently dealing a bug due to my current setup that means binary_piecewise still exists and get_model() calls are trying to use it rather than going via binary_bt like it should now.

poneill129 · 2023-09-14T09:00:21Z

Hi all, more updates on the piecewise model (this time on the testing suite side of things). The tests appear to pass in my local version of this, it is only in the CI tests that tests for other functions begin failing. I haven't found a way to narrow what it is that's causing it, but I have a feeling there is something lingering after the piecewise tests are done.

I don't know if its possible for this to be the reason, but if its not I cannot see why the test is failing since it shouldn't be affected by any of the changes I've made.

And to ease nervous minds, I've painstakingly looked through each of the tests, and done some quick fitting using pulsar data, to verify the model and tests are still behaving as intended (i.e. everything is not broken!)

dlakaplan · 2023-09-14T15:36:59Z

Hi, Patrick. The CI failure that I saw was not something you did. The test was for something else and was close to passing, but occasionally it fails because the tolerances may be too tight. I just tried to re-run it.

abhisrkckl · 2023-09-26T20:42:23Z

Hi @poneill129, from reading the comments above, it seems that this PR is pretty much ready. Are you planning to do anything more in this PR?

poneill129 · 2023-09-26T21:14:22Z

Not at the moment, happy with it. But if anything else needs clearing up I'm able to make changes.

Updated to refelct comments

5983c2e

poneill129 changed the title ~~Updated to reflect comments~~ Updated piecewise orbital model Mar 31, 2023

Merge branch 'master' into piecewise_orbital_model

5203147

dlakaplan reviewed Sep 7, 2023

View reviewed changes

src/pint/models/binary_piecewise.py Outdated Show resolved Hide resolved

dlakaplan reviewed Sep 7, 2023

View reviewed changes

src/pint/models/binary_piecewise.py Outdated Show resolved Hide resolved

dlakaplan reviewed Sep 7, 2023

View reviewed changes

src/pint/models/binary_piecewise.py Outdated Show resolved Hide resolved

dlakaplan reviewed Sep 7, 2023

View reviewed changes

src/pint/models/binary_piecewise.py Outdated Show resolved Hide resolved

dlakaplan reviewed Sep 7, 2023

View reviewed changes

src/pint/models/binary_piecewise.py Outdated Show resolved Hide resolved

dlakaplan reviewed Sep 7, 2023

View reviewed changes

src/pint/models/binary_piecewise.py Outdated Show resolved Hide resolved

dlakaplan reviewed Sep 7, 2023

View reviewed changes

src/pint/models/binary_piecewise.py Outdated Show resolved Hide resolved

dlakaplan reviewed Sep 7, 2023

View reviewed changes

src/pint/models/binary_piecewise.py Outdated Show resolved Hide resolved

dlakaplan reviewed Sep 7, 2023

View reviewed changes

David's suggested changes

f1132d4

poneill129 and others added 10 commits September 9, 2023 23:48

Blacken code plus minor fixes to last change

bce039c

Cleaning up the test suite

03894b2

Merge branch 'piecewise_orbital_model' into merging_bt_pw

eb01191

Merge branch 'nanograv:master' into piecewise_orbital_model

83ba865

Getting up-to-date

ca474f2

Another round of bug fixing

0556b2e

Blacken code

1b805d4

bug-fixing in setup_internal_structures

ca70953

Commented failing tests

48294a7

Blacken code

484508e

abhisrkckl merged commit 01d3153 into nanograv:master Sep 28, 2023
8 checks passed

abhisrkckl mentioned this pull request Oct 4, 2023

Piecewise orbit model #1378

Closed

abhisrkckl mentioned this pull request Oct 19, 2023

Wishlist for PINT development #1576

Open

Updated piecewise orbital model #1545

Updated piecewise orbital model #1545

Conversation

poneill129 commented Mar 31, 2023

codecov bot commented Mar 31, 2023 • edited Loading

Codecov Report

dlakaplan commented Apr 6, 2023

scottransom commented Jun 14, 2023

poneill129 commented Jun 14, 2023

scottransom commented Sep 5, 2023

poneill129 commented Sep 7, 2023

Choose a reason for hiding this comment

dlakaplan commented Sep 7, 2023

poneill129 commented Sep 7, 2023

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

dlakaplan commented Sep 7, 2023

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

dlakaplan commented Sep 7, 2023

poneill129 commented Sep 7, 2023 • edited Loading

dlakaplan commented Sep 7, 2023

poneill129 commented Sep 8, 2023

poneill129 commented Sep 8, 2023

poneill129 commented Sep 14, 2023

dlakaplan commented Sep 14, 2023

abhisrkckl commented Sep 26, 2023

poneill129 commented Sep 26, 2023

codecov bot commented Mar 31, 2023 •

edited

Loading

poneill129 commented Sep 7, 2023 •

edited

Loading