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.

Sign up for GitHub

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

Jump to bottom

Use TRFLSQFitter instead of LevMarLSQFitter #3202

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Use TRFLSQFitter instead of LevMarLSQFitter #3202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2033c57
Use TRFLSQFitter instead of LevMarLSQFitter
pllim Sep 25, 2024
2eae18c
Copy updated blackbody test code from astropy
pllim Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,17 @@ Cubeviz
- Custom Spectrum1D writer format ``jdaviz-cube`` is removed. Use ``wcs1d-fits`` from
``specutils`` instead. [#2094]

- Aperture Photometry plugin now uses TRFLSQFitter to fit radial profile because LevMarLSQFitter
is no longer recommended by Astropy. [#3202]

Imviz
^^^^^

- Deprecated Rotate Canvas plugin was removed; use Orientation plugin instead. [#2878]

- Aperture Photometry plugin now uses TRFLSQFitter to fit radial profile because LevMarLSQFitter
is no longer recommended by Astropy. [#3202]

Mosviz
^^^^^^

Expand Down
27 changes: 11 additions & 16 deletions jdaviz/configs/default/plugins/model_fitting/tests/test_blackbody.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,16 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
# copied and modified from astropy: https://github.com/astropy/astropy/pull/12304
"""Tests for physical functions."""
# pylint: disable=no-member, invalid-name
"""Tests for blackbody functions."""
import pytest
import numpy as np

from jdaviz.models import BlackBody
from astropy.modeling.fitting import LevMarLSQFitter

from astropy.tests.helper import assert_quantity_allclose
from astropy import units as u
from astropy.modeling.fitting import TRFLSQFitter
from astropy.tests.helper import assert_quantity_allclose
from astropy.utils.compat.optional_deps import HAS_SCIPY
from astropy.utils.exceptions import AstropyUserWarning
from astropy.utils.compat.optional_deps import HAS_SCIPY # noqa


__doctest_skip__ = ["*"]


# BlackBody tests
from jdaviz.models import BlackBody


@pytest.mark.parametrize("temperature", (3000 * u.K, 2726.85 * u.deg_C))
Expand Down Expand Up @@ -61,10 +54,12 @@ def test_blackbody_return_units():
assert b(1.0 * u.micron).unit == u.MJy / u.sr


@pytest.mark.skipif("not HAS_SCIPY")
@pytest.mark.skipif(not HAS_SCIPY, reason="requires scipy")
def test_blackbody_fit():

fitter = LevMarLSQFitter()
fitter = TRFLSQFitter()
rtol = 0.54
atol = 1e-15

b = BlackBody(3000 * u.K, scale=5e-17, output_units=u.Jy / u.sr)

Expand All @@ -73,8 +68,8 @@ def test_blackbody_fit():

b_fit = fitter(b, wav, fnu, maxiter=1000)

assert_quantity_allclose(b_fit.temperature, 2840.7438355865065 * u.K)
assert_quantity_allclose(b_fit.scale, 5.803783292762381e-17)
assert_quantity_allclose(b_fit.temperature, 2840.7438355865065 * u.K, rtol=rtol)
assert_quantity_allclose(b_fit.scale, 5.803783292762381e-17, atol=atol)


def test_blackbody_overflow():
Expand Down
4 changes: 2 additions & 2 deletions jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import numpy as np
from astropy import units as u
from astropy.modeling.fitting import LevMarLSQFitter
from astropy.modeling.fitting import TRFLSQFitter
from astropy.modeling import Parameter
from astropy.modeling.models import Gaussian1D
from astropy.time import Time
Expand Down Expand Up @@ -876,7 +876,7 @@ def calculate_photometry(self, dataset=None, aperture=None, background=None,

# Fit Gaussian1D to radial profile data.
if self.fit_radial_profile:
fitter = LevMarLSQFitter()
fitter = TRFLSQFitter()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larrybradley , is this replacement reasonable or another one is better? Also see astropy/photutils#1895

Copy link
Member

Choose a reason for hiding this comment

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

That should work fine. I don't think you are using parameter bounds, so LMLSQFitter is also an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Well, not yet. No idea if that is going to be future request or not. 😬

Copy link
Member

Choose a reason for hiding this comment

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

is there any consequence on performance between the two for this use-case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing that we have to worry about. I am using what Astropy now suggests in their docs. Tom R made that change upstream and he has been also working on modeling performance there, so he wouldn't recommend anything that would make things worse.

Copy link
Member

Choose a reason for hiding this comment

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

In my testing, TRFLSQFitter is about 4-5% slower than LevMarLSQFitter. However, this difference will not be noticeable here.

y_max = y_data.max()
x_mean = x_data[np.where(y_data == y_max)].mean()
std = 0.5 * (phot_table['semimajor_sigma'][0] +
Expand Down
Loading