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

Sensibly handle unfittable params, No unfittable params in plk #1648

Merged
merged 12 commits into from
Oct 17, 2023

Conversation

abhisrkckl
Copy link
Contributor

Unfittable parameters are parameters for which no derivatives are defined so that they cannot be fit.

  1. Fitter objects should not be created any unfittable parameters are unfrozen. Currently, it fails with an unclear error message while calling fit_toas().
  2. TimingModel.designmatrix() should fail with an informative error message upon encountering free unfittable parameters. Currently, it fails with an unclear error message.
  3. pintk interface shouldn't list unfittable parameters. This will help unclutter pintk.

@abhisrkckl abhisrkckl linked an issue Oct 4, 2023 that may be closed by this pull request
@abhisrkckl abhisrkckl changed the title Sensibly handle unfittable params WIP: Sensibly handle unfittable params Oct 4, 2023
@dlakaplan
Copy link
Contributor

The errors here might be due to function parameters which are visible but not intrinsically part of the model. So maybe just a check for parameter type as well.

@abhisrkckl
Copy link
Contributor Author

The errors here might be due to function parameters which are visible but not intrinsically part of the model. So maybe just a check for parameter type as well.

A subset of the errors were indeed indirectly related to funcParameters. But they were actually due to a bug in the ELL1k model which I have fixed.

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (9e21a05) 68.34% compared to head (fb14307) 68.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1648      +/-   ##
==========================================
- Coverage   68.34%   68.34%   -0.01%     
==========================================
  Files         104      104              
  Lines       24101    24112      +11     
  Branches     4277     4280       +3     
==========================================
+ Hits        16473    16480       +7     
- Misses       6545     6547       +2     
- Partials     1083     1085       +2     
Files Coverage Δ
src/pint/models/binary_ell1.py 86.18% <100.00%> (+0.18%) ⬆️
src/pint/pintk/plk.py 13.51% <ø> (ø)
src/pint/fitter.py 83.63% <0.00%> (-0.21%) ⬇️
src/pint/models/timing_model.py 83.82% <50.00%> (-0.13%) ⬇️

... and 3 files with indirect coverage changes

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

@abhisrkckl abhisrkckl changed the title WIP: Sensibly handle unfittable params Sensibly handle unfittable params, No unfittable params in plk Oct 10, 2023
@abhisrkckl
Copy link
Contributor Author

Plk interface before excluding unfittable params:
image

After:
image

@dlakaplan
Copy link
Contributor

This looks good. Is it ready?

@abhisrkckl
Copy link
Contributor Author

Yes.

@dlakaplan
Copy link
Contributor

Should I merge this?

@scottransom
Copy link
Member

Sounds good to me!

@dlakaplan dlakaplan merged commit 0201737 into nanograv:master Oct 17, 2023
8 checks passed
@abhisrkckl abhisrkckl deleted the fittable-params branch May 14, 2024 08:44
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.

Only fittable parameters should have "fit" check boxes in pintk
3 participants