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

FDJUMP implementation (Hacky version) #1615

Merged
merged 25 commits into from
Aug 24, 2023

Conversation

abhisrkckl
Copy link
Contributor

@abhisrkckl abhisrkckl commented Aug 4, 2023

This PR implements the system-dependent FD parameters (FDJUMPs).
(See #1480)

In PINT, maskParameters and prefixParameters have identical naming schemes. This makes it very challenging to implement FDJUMPs, which have properties of both maskParameters and prefixParameters.

So I am doing a stopgap measure here, by renaming "FDJUMPn" --> "FDnJUMP" so that the mask and prefix indices don't interfere with each other. This also means that we would need an upper bound on the number of allowed prefix indices (n). I have set this to 20. This is controlled by the pint.models.fdjump.fdjump_max_index variable.

The above choice also leads to an inconsistency with tempo2 due to the parameter name being "FDnJUMP" instead of "FDJUMPn". I have added a preprocessing step to ensure that the tempo2 par files with "FDJUMPn" are read correctly. In addition, tempo2-compatible par files with "FDJUMPn" can be written out using the format="tempo2" option.

A proper solution to this will be to overhaul the maskParameter naming scheme (e.g. #1059). However, I am not doing this here since we need this quickly for the IPTA EDR3.

I have also put a boolean parameter FDJUMPLOG to control whether the FDjumps act on the log-frequency or the frequency. This is False by default to make it consistent with tempo2. Note that regular FD parameters act on the log-frequency .

@abhisrkckl
Copy link
Contributor Author

@rossjjennings @gooddc Can you try this and see if it does what you need?

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Patch coverage: 90.36% and project coverage change: +0.09% 🎉

Comparison is base (ef2b0b3) 68.15% compared to head (6a73c83) 68.25%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1615      +/-   ##
==========================================
+ Coverage   68.15%   68.25%   +0.09%     
==========================================
  Files          98       99       +1     
  Lines       22702    22784      +82     
  Branches     3904     3916      +12     
==========================================
+ Hits        15473    15551      +78     
- Misses       6270     6273       +3     
- Partials      959      960       +1     
Files Changed Coverage Δ
src/pint/models/frequency_dependent.py 83.09% <0.00%> (ø)
src/pint/models/fdjump.py 89.85% <89.85%> (ø)
src/pint/models/__init__.py 100.00% <100.00%> (ø)
src/pint/models/model_builder.py 95.50% <100.00%> (+0.21%) ⬆️

... and 1 file with indirect coverage changes

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

@abhisrkckl abhisrkckl linked an issue Aug 8, 2023 that may be closed by this pull request
@gooddc
Copy link

gooddc commented Aug 14, 2023

The changes don't seem to fully work with timing_model.py

Trying to set the list of free_params equal to a list including FD1JUMP returns the following error:

UnknownParameter Traceback (most recent call last)
Cell In[11], line 2
1 # Set free params based on list in the config file (want to update JUMP handling differently soon)
----> 2 fo.model.free_params = tc.get_free_params(fo)
4 # Do the fit
5 try:

File ~/mambaforge/envs/IPTA_Env_dev_version/lib/python3.9/site-packages/pint/models/timing_model.py:566, in TimingModel.free_params(self, params)
564 @free_params.setter
565 def free_params(self, params):
--> 566 params_true = {self.match_param_aliases(p) for p in params}
567 for p in self.params:
568 getattr(self, p).frozen = p not in params_true

File ~/mambaforge/envs/IPTA_Env_dev_version/lib/python3.9/site-packages/pint/models/timing_model.py:566, in (.0)
564 @free_params.setter
565 def free_params(self, params):
--> 566 params_true = {self.match_param_aliases(p) for p in params}
567 for p in self.params:
568 getattr(self, p).frozen = p not in params_true

File ~/mambaforge/envs/IPTA_Env_dev_version/lib/python3.9/site-packages/pint/models/timing_model.py:603, in TimingModel.match_param_aliases(self, alias)
600 continue
601 return pint_par
--> 603 raise UnknownParameter(f"{alias} is not recognized as a parameter or alias")

UnknownParameter: FD1JUMP is not recognized as a parameter or alias

@abhisrkckl
Copy link
Contributor Author

@goodc Can you try with "FD1JUMP1" instead of "FD1JUMP"?

Does this usually work with other mask parameters? What happens when you set model.free_params to ["JUMP"] instead of ["JUMP1"]?

@gooddc
Copy link

gooddc commented Aug 14, 2023

Okay, I didn't realize it needed to be FD1JUMP1 not FD1JUMP. JUMP doesn't work: only JUMP1, JUMP2, etc. For multiple FDJUMPs will the formatting be FD1JUMP1, FD1JUMP2, FD1JUMP3 or FD1JUMP1, FD1JUMP2, FD1JUMP3?

@abhisrkckl
Copy link
Contributor Author

abhisrkckl commented Aug 14, 2023

For multiple FDJUMPs will the formatting be FD1JUMP1, FD1JUMP2, FD1JUMP3 or FD1JUMP1, FD1JUMP2, FD1JUMP3?

For multiple FD1JUMPs it will be.FD1JUMP1, FD1JUMP2, etc.
For multiple FD2JUMPs it will be.FD2JUMP1, FD2JUMP2, etc.
and so on.

In FDxJUMPy, x is the FD index (power of the radio frequency in the FD delay expression), and y is the mask index.

@gooddc
Copy link

gooddc commented Aug 14, 2023

Ah okay. I'm not sure I fully understand how this compares to the way that tempo2 is doing this. Does that mean that FD1JUMPn is tied to FD1, FD2JUMPn is tied to FD2, etc.?

@abhisrkckl
Copy link
Contributor Author

abhisrkckl commented Aug 14, 2023

Does that mean that FD1JUMPn is tied to FD1, FD2JUMPn is tied to FD2, etc.?

Yes.

There is an extra complication though. In tempo2, FDx delay is proportional to (log(freq/1_GHz))**x, whereas FDJUMPx (FDxJUMP in PINT) is proportional to (freq/1_GHz)**x without the logarithm. You can control this in PINT using the FDJUMPLOG parameter, which is not there in tempo2. It is set to be tempo2-compatible by default.

Caveat
------
`FDJUMP`s have two indices: the polynomial/FD/prefix index and the system/mask
index. i.e., they have properties of both `maskParameter`s such as `JUMP`s and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be ":class:~pint.models.parameter.maskParameter" and similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work properly in a note box.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. In that case I think still using double back ticks is better. Otherwise the formatting gets confused.

@scottransom scottransom merged commit 910bb0c into nanograv:master Aug 24, 2023
@abhisrkckl abhisrkckl deleted the fdjump-ugly branch May 14, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tag-specific FD parameters
4 participants