Added GeneralizedLorentz1D and SmoothBrokenPowerLaw classes to stingray.simulator.models.
#889
Conversation
ankitkhushwaha
force-pushed
the
jacob
branch
3 times, most recently
from
85f0175 to
8026b67
Compare
|
I think changes are ready for review. |
|
@ankitkhushwaha thanks for your PR. However, I haven't noticed any tests failing, there are no issues open about it as far as I can tell. Could you be clearer about what is failing, and what your changes do? |
|
@matteobachetti these test are fixed now. TODO: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #889 +/- ##
===========================================
- Coverage 96.04% 82.70% -13.34%
===========================================
Files 48 48
Lines 9789 9871 +82
===========================================
- Hits 9402 8164 -1238
- Misses 387 1707 +1320 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hey @matteobachetti, mostly this pr is ready to be reviewed, could you review this. |
|
Hey @matteobachetti , all TODO are completed. |
ankitkhushwaha
changed the title
GeneralizedLorentz1D, SmoothBrokenPowerLaw in stingray.simulator.modelsGeneralizedLorentz1D and SmoothBrokenPowerLaw classes to stingray.simulator.models.
|
@ankitkhushwaha just to clarify: have you suppressed the use of the derivative in all model fits? A lot of lines seem not to be covered by tests |
|
|
||
| data = model_with_deriv(x) + n | ||
| fitter_with_deriv = fitting.LevMarLSQFitter() | ||
| new_model_with_deriv = fitter_with_deriv(model_with_deriv, x, data) |
There was a problem hiding this comment.
under the hood this tests exeutes the fit_deriv method see
https://docs.astropy.org/en/stable/api/astropy.modeling.fitting.LevMarLSQFitter.html -> estimate_jacobian
There was a problem hiding this comment.
i have seen these types of tests in astropy codebase
so i implemented this
There was a problem hiding this comment.
What I mean is that codecov is showing a lot of code not being executed during tests, including, e.g., the fit_deriv and bounding_box methods of GeneralizedLorentz1D (and even fewer methods of the broken power law model). I don't know why, it might be a bug of codecov, but I suggest to verify that all code is actually executed
| @pytest.mark.parametrize( | ||
| "model, x_lim", | ||
| [ | ||
| (models.SmoothBrokenPowerLaw(1, -2, 2, 10), [0.01, 70]), |
There was a problem hiding this comment.
also if the boundaries are too much large ex [0.01, 100]
the fitter in its internal calculaltion produces infinity. and fitting fails
so i changed boundaries to [0.01, 70]
| ---------- | ||
| factor : float | ||
| The multiple of FWHM used to define the limits. | ||
| Default is chosen to include most (99%) of the |
There was a problem hiding this comment.
25 times the fwhm seems a lot more than 99%?
|
|
||
| data = model_with_deriv(x) + n | ||
| fitter_with_deriv = fitting.LevMarLSQFitter() | ||
| new_model_with_deriv = fitter_with_deriv(model_with_deriv, x, data) |
There was a problem hiding this comment.
What I mean is that codecov is showing a lot of code not being executed during tests, including, e.g., the fit_deriv and bounding_box methods of GeneralizedLorentz1D (and even fewer methods of the broken power law model). I don't know why, it might be a bug of codecov, but I suggest to verify that all code is actually executed
…ingray.simulator.models`
|
will surely check it if lines are being tested or not. |
TODO:
GeneralizedLorentz1DandSmoothBrokenPowerLaw, tostingray.simulator.models.i have confirmed that jacobian for function with respect to each parameter is correct using sympy
@matteobachetti