Repository: spacetelescope/synphot_refactor

Issue:
Operations with Box1D spectral element significantly slower than with GaussianFlux1D 
<!-- This comments are hidden when you submit the issue,
so you do not need to remove them! -->

<!-- Please be sure to check out our code of conduct,
https://github.com/spacetelescope/synphot_refactor/blob/master/CODE_OF_CONDUCT.md . -->

<!-- Please have a search on our GitHub repository to see if a similar
issue has already been posted.
If a similar issue is closed, have a quick look to see if you are satisfied
by the resolution.
If not please go ahead and open an issue! -->

<!-- Please check that the development version still produces the same bug.
You can install development version with
pip install git+https://github.com/spacetelescope/synphot_refactor
command. -->

### Description
<!-- Provide a general description of the bug. -->
Not entirely sure if this is a real issue or I'm misunderstanding some synphot internals, but I have noticed that Observation operations when using a bandpass based on Box1D are systematically slower than equivalent operations with a bandpass based on a Gaussian element. 

### Expected behavior
<!-- What did you expect to happen. -->
Both sets of operations to take approximately the same amounts of time.

### Actual behavior
<!-- What actually happened. -->
<!-- Was the output confusing or poorly described? -->

### Steps to Reproduce
<!-- Ideally a code example could be provided so we can run it ourselves. -->
<!-- If you are pasting code, use triple backticks (```) around
your code snippet. -->
<!-- If necessary, sanitize your screen output to be pasted so you do not
reveal secrets like tokens and passwords. -->

MWE example below. Specifying integration_type has no effect in either case. 

```python
from synphot import Observation, SourceSpectrum, SpectralElement, units
from synphot.models import BlackBodyNorm1D, GaussianFlux1D, Box1D
import numpy as np
s = SourceSpectrum(BlackBodyNorm1D, temperature=5000)
l = s.avgwave()
BW = l*0.1
FWHM = BW*np.sqrt(np.log(2)/np.pi)*2
box = SpectralElement(Box1D, x_0=l, width=BW)
gauss = SpectralElement(GaussianFlux1D, mean=l, fwhm=FWHM)

obs_box = Observation(s, box)
obs_gauss = Observation(s, gauss)

flux_box = obs_box.integrate()
flux_gauss = obs_gauss.integrate()
print(flux_box - flux_gauss) # -0.0006330283047517593 ph / (cm2 s)
```

Timing tests with ipython on two different systems:
```
%timeit obs_box = Observation(s, box)
#sys 1: 458 ms ± 43.7 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
#sys 2: 106 ms ± 368 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

%timeit obs_gauss = Observation(s, gauss)
#sys 1: 62.8 ms ± 2.57 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)
#sys 2: 5.77 ms ± 16.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

%timeit flux_box = obs_box.integrate()
#sys 1: 265 ms ± 22.3 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
#sys 2: 27.1 ms ± 250 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

%timeit flux_gauss = obs_gauss.integrate()
#sys 1: 48 ms ± 5.31 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)
#sys 2: 4.33 ms ± 42 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```

### System Details
<!-- Even if you do not think this is necessary, it is useful information for the maintainers.
Please run the following snippet and paste the output below:
import platform; print(platform.platform())
import sys; print("Python", sys.version)
import numpy; print("Numpy", numpy.__version__)
import astropy; print("astropy", astropy.__version__)
import scipy; print("Scipy", scipy.__version__)
import matplotlib; print("Matplotlib", matplotlib.__version__)
import synphot; print("Synphot", synphot.__version__)
-->

sys1: 
macOS-11.7-x86_64-i386-64bit
Python 3.10.8 (main, Oct 12 2022, 02:49:24) [Clang 13.0.0 (clang-1300.0.29.3)]
Numpy 1.23.4
astropy 5.1
Scipy 1.8.1
Matplotlib 3.5.3
Synphot 1.1.1

sys2:
Linux-4.15.0-176-generic-x86_64-with-glibc2.27
Python 3.10.2 (main, Feb 22 2022, 10:06:51) [GCC 7.5.0]
Numpy 1.22.2
astropy 5.0.1
Scipy 1.8.0
Matplotlib 3.5.1
Synphot 1.1.1

Discussion:
pllim: Please also add a change log entry in https://github.com/spacetelescope/synphot_refactor/blob/master/CHANGES.rst . Thanks!
pllim: Weird that CI didn't kick off. Let me close/reopen and see.
codecov[bot]: # [Codecov](https://codecov.io/gh/spacetelescope/synphot_refactor/pull/342?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) Report
Base: **97.08**% // Head: **97.09**% // Increases project coverage by **`+0.00%`** :tada:
> Coverage data is based on head [(`7cc2590`)](https://codecov.io/gh/spacetelescope/synphot_refactor/pull/342?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) compared to base [(`a5fa653`)](https://codecov.io/gh/spacetelescope/synphot_refactor/commit/a5fa6533bdcb58bb43d8ee43526d15c9817292bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope).
> Patch coverage: 100.00% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##           master     #342   +/-   ##
=======================================
  Coverage   97.08%   97.09%           
=======================================
  Files          17       17           
  Lines        2024     2029    +5     
=======================================
+ Hits         1965     1970    +5     
  Misses         59       59           
```


| [Impacted Files](https://codecov.io/gh/spacetelescope/synphot_refactor/pull/342?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | |
|---|---|---|
| [synphot/models.py](https://codecov.io/gh/spacetelescope/synphot_refactor/pull/342/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope#diff-c3lucGhvdC9tb2RlbHMucHk=) | `95.82% <100.00%> (+0.06%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/spacetelescope/synphot_refactor/pull/342?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope).

pllim: Oh no, I think we got hit by tox 4.0 too.
pllim: Hello. Can you please rebase to pick up https://github.com/spacetelescope/synphot_refactor/pull/343 ? Thanks!
pllim: Thank you! LGTM now but I need to know that stakeholders like @vglaidler, @ariedel, and co. would be okay with this. In the meantime, I ask for your patience. 😸 

Patch:
diff --git a/.github/workflows/ci_workflows.yml b/.github/workflows/ci_workflows.yml
index 733bcc1..d97779e 100644
--- a/.github/workflows/ci_workflows.yml
+++ b/.github/workflows/ci_workflows.yml
@@ -83,7 +83,7 @@ jobs:
             toxposargs: --open-files
 
           - name: Test with old dependencies
-            os: ubuntu-18.04
+            os: ubuntu-20.04
             python: 3.8
             toxenv: py38-test-oldestdeps
             toxposargs: --open-files
@@ -112,8 +112,7 @@ jobs:
         python-version: ${{ matrix.python }}
     - name: Install Python dependencies
       run: |
-        python -m pip install --upgrade pip
-        python -m pip install "tox<4"
+        python -m pip install --upgrade pip tox
     - name: Run tests
       run: tox -e ${{ matrix.toxenv }} -- ${{ matrix.toxposargs }}
     - name: Upload coverage to codecov
diff --git a/.github/workflows/predeps_workflow.yml b/.github/workflows/predeps_workflow.yml
index 85fae24..4f82683 100644
--- a/.github/workflows/predeps_workflow.yml
+++ b/.github/workflows/predeps_workflow.yml
@@ -46,7 +46,6 @@ jobs:
         python-version: ${{ matrix.python }}
     - name: Install Python dependencies
       run: |
-        python -m pip install --upgrade pip
-        python -m pip install "tox<4"
+        python -m pip install --upgrade pip tox
     - name: Run tests
       run: tox -e ${{ matrix.toxenv }} -- ${{ matrix.toxposargs }}
diff --git a/CHANGES.rst b/CHANGES.rst
index 6194193..609339b 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -12,6 +12,10 @@
 
 - OBMAG and VEGAMAG are no longer interchangeable. [#331]
 
+- ``Box1D`` model now takes optional ``step`` input to allow user control
+  over the generated sampleset. Default behavior maintains backwards 
+  compatibility. [#342]
+
 1.1.1 (2021-11-18)
 ==================
 
diff --git a/synphot/binning.py b/synphot/binning.py
index cca5313..9577064 100644
--- a/synphot/binning.py
+++ b/synphot/binning.py
@@ -8,7 +8,7 @@ import numpy as np
 from astropy import units as u
 
 # LOCAL
-from . import exceptions
+from synphot import exceptions
 
 __all__ = ['calculate_bin_edges', 'calculate_bin_widths',
            'calculate_bin_centers', 'wave_range', 'pixel_range']
@@ -41,7 +41,7 @@ def _slow_calcbinflux(len_binwave, i_beg, i_end, avflux, deltaw):
 # Try to import the C version of calcbinflux, otherwise fall back
 # to the Python implementation above.
 try:
-    from . import synphot_utils
+    from synphot import synphot_utils
 except ImportError:
     calcbinflux = _slow_calcbinflux
 else:
diff --git a/synphot/blackbody.py b/synphot/blackbody.py
index d8bc5fe..ec45ec9 100644
--- a/synphot/blackbody.py
+++ b/synphot/blackbody.py
@@ -22,7 +22,7 @@ from astropy.modeling.core import Fittable1DModel
 from astropy.modeling.parameters import Parameter
 from astropy.utils.exceptions import AstropyUserWarning
 
-from .units import FNU, FLAM
+from synphot.units import FNU, FLAM
 
 __all__ = ['BlackBody1D', 'blackbody_nu', 'blackbody_lambda']
 
diff --git a/synphot/models.py b/synphot/models.py
index a2a3b00..2a2cc0a 100644
--- a/synphot/models.py
+++ b/synphot/models.py
@@ -22,10 +22,10 @@ from astropy.utils import metadata
 from astropy.utils.exceptions import AstropyUserWarning
 
 # LOCAL
-from . import units
-from .compat import ASTROPY_LT_5_0
-from .exceptions import SynphotError
-from .utils import merge_wavelengths
+from synphot import units
+from synphot.compat import ASTROPY_LT_5_0
+from synphot.exceptions import SynphotError
+from synphot.utils import merge_wavelengths
 
 __all__ = ['BlackBody1D', 'BlackBodyNorm1D', 'Box1D', 'ConstFlux1D',
            'Empirical1D', 'Gaussian1D', 'GaussianAbsorption1D',
@@ -185,7 +185,22 @@ class Box1D(_models.Box1D):
     """Same as `astropy.modeling.functional_models.Box1D`, except with
     ``sampleset`` defined.
 
+    Parameters
+    ----------
+    step : float
+        Distance of first and last points w.r.t. bounding box. In default units
+        (nominally Angstrom). Defaults to 0.01
+
     """
+    def __init__(self, *args, **kwargs):
+
+        if "step" in kwargs:
+            self.step = kwargs.pop("step")
+        else:
+            self.step = 0.01
+
+        super(Box1D, self).__init__(*args, **kwargs)
+
     @staticmethod
     def _calc_sampleset(w1, w2, step, minimal):
         """Calculate sampleset for each model."""
@@ -196,19 +211,23 @@ class Box1D(_models.Box1D):
 
         return arr
 
-    def sampleset(self, step=0.01, minimal=False):
+    def sampleset(self, step=None, minimal=False):
         """Return ``x`` array that samples the feature.
 
         Parameters
         ----------
-        step : float
-            Distance of first and last points w.r.t. bounding box.
+        step : float, optional
+            Distance of first and last points w.r.t. bounding box. If None,
+            set to attribute ``step``.
 
         minimal : bool
             Only return the minimal points needed to define the box;
             i.e., box edges and a point outside on each side.
 
         """
+        if step is None:
+            step = self.step
+
         if ASTROPY_LT_5_0:
             w1, w2 = self.bounding_box
         else:
diff --git a/synphot/observation.py b/synphot/observation.py
index 14e4477..b90546b 100644
--- a/synphot/observation.py
+++ b/synphot/observation.py
@@ -18,9 +18,10 @@ from astropy.utils.exceptions import (AstropyUserWarning,
                                       AstropyDeprecationWarning)
 
 # LOCAL
-from . import binning, exceptions, units, utils
-from .models import Empirical1D
-from .spectrum import BaseSourceSpectrum, SourceSpectrum, SpectralElement
+from synphot import binning, exceptions, units, utils
+from synphot.models import Empirical1D
+from synphot.spectrum import (BaseSourceSpectrum, SourceSpectrum,
+                              SpectralElement)
 
 __all__ = ['Observation']
 
diff --git a/synphot/reddening.py b/synphot/reddening.py
index 81c055c..6ee9b9f 100644
--- a/synphot/reddening.py
+++ b/synphot/reddening.py
@@ -9,11 +9,11 @@ import numpy as np
 from astropy import units as u
 
 # LOCAL
-from . import exceptions, specio, units
-from .compat import HAS_DUST_EXTINCTION
-from .config import Conf
-from .models import Empirical1D
-from .spectrum import BaseUnitlessSpectrum
+from synphot import exceptions, specio, units
+from synphot.compat import HAS_DUST_EXTINCTION
+from synphot.config import Conf
+from synphot.models import Empirical1D
+from synphot.spectrum import BaseUnitlessSpectrum
 
 __all__ = ['ExtinctionModel1D', 'ReddeningLaw', 'ExtinctionCurve',
            'etau_madau']
diff --git a/synphot/specio.py b/synphot/specio.py
index 8d93aa9..925f98b 100644
--- a/synphot/specio.py
+++ b/synphot/specio.py
@@ -16,8 +16,7 @@ from astropy.utils.data import get_readable_fileobj
 from astropy.utils.exceptions import AstropyUserWarning
 
 # LOCAL
-from . import exceptions, units
-from synphot import __version__
+from synphot import __version__, exceptions, units
 
 __all__ = ['read_remote_spec', 'read_spec', 'read_ascii_spec',
            'read_fits_spec', 'write_fits_spec']
diff --git a/synphot/spectrum.py b/synphot/spectrum.py
index 75057d5..4a895c3 100644
--- a/synphot/spectrum.py
+++ b/synphot/spectrum.py
@@ -20,9 +20,9 @@ from astropy.utils.exceptions import AstropyUserWarning
 from astropy.utils import metadata
 
 # LOCAL
-from . import exceptions, specio, units, utils
-from .config import Conf, conf
-from .models import ConstFlux1D, Empirical1D, get_waveset, get_metadata
+from synphot import exceptions, specio, units, utils
+from synphot.config import Conf, conf
+from synphot.models import ConstFlux1D, Empirical1D, get_waveset, get_metadata
 
 __all__ = ['BaseSpectrum', 'BaseSourceSpectrum', 'SourceSpectrum',
            'BaseUnitlessSpectrum', 'SpectralElement']
@@ -874,7 +874,7 @@ class BaseSpectrum:
             ``specutils`` is not installed.
 
         """
-        from .compat import HAS_SPECUTILS
+        from synphot.compat import HAS_SPECUTILS
 
         if not HAS_SPECUTILS:  # pragma: no cover
             raise ImportError('specutils must be installed to use this method')
diff --git a/synphot/thermal.py b/synphot/thermal.py
index 2ccb1e2..093d412 100644
--- a/synphot/thermal.py
+++ b/synphot/thermal.py
@@ -6,9 +6,9 @@ from astropy import units as u
 from astropy.io import fits
 
 # LOCAL
-from . import exceptions, specio, units
-from .models import BlackBody1D, Empirical1D
-from .spectrum import BaseUnitlessSpectrum, SourceSpectrum
+from synphot import exceptions, specio, units
+from synphot.models import BlackBody1D, Empirical1D
+from synphot.spectrum import BaseUnitlessSpectrum, SourceSpectrum
 
 __all__ = ['ThermalSpectralElement']
 
diff --git a/synphot/units.py b/synphot/units.py
index e9ba208..e3c4f9f 100644
--- a/synphot/units.py
+++ b/synphot/units.py
@@ -106,7 +106,7 @@ def spectral_density_count(wav, area):
         List of equivalencies.
 
     """
-    from .binning import calculate_bin_widths, calculate_bin_edges
+    from synphot.binning import calculate_bin_widths, calculate_bin_edges
 
     wav = wav.to(u.AA, equivalencies=u.spectral())
     area = area.to(AREA)
@@ -221,7 +221,7 @@ def _convert_flux(wavelengths, fluxes, out_flux_unit, area=None,
 
     # VEGAMAG
     if VEGAMAG.to_string() in flux_unit_names:
-        from .spectrum import SourceSpectrum
+        from synphot.spectrum import SourceSpectrum
 
         if not isinstance(vegaspec, SourceSpectrum):
             raise exceptions.SynphotError('Vega spectrum is missing.')
diff --git a/synphot/utils.py b/synphot/utils.py
index dae512c..99cb398 100644
--- a/synphot/utils.py
+++ b/synphot/utils.py
@@ -14,7 +14,7 @@ from astropy.config import ConfigItem
 from astropy.utils.data import download_file
 
 # LOCAL
-from . import exceptions, units
+from synphot import exceptions, units
 
 __all__ = ['overlap_status', 'validate_totalflux', 'validate_wavelengths',
            'generate_wavelengths', 'merge_wavelengths', 'download_data']
@@ -289,7 +289,7 @@ def download_data(path_root, verbose=True, dry_run=False):
         A list of downloaded files.
 
     """
-    from .config import conf  # Avoid potential circular import
+    from synphot.config import conf  # Avoid potential circular import
     BASE_HOST = 'https://ssb.stsci.edu/trds/'
 
     if path_root is not None:
diff --git a/tox.ini b/tox.ini
index c8ea5b8..b26058a 100644
--- a/tox.ini
+++ b/tox.ini
@@ -8,13 +8,11 @@ envlist =
 requires =
     setuptools >= 30.3.0
     pip >= 19.3.1
-    tox < 4
-isolated_build = true
 
 [testenv]
 
 # Pass through the following environemnt variables which are needed for the CI
-passenv = HOME WINDIR CC CI
+passenv = HOME,WINDIR,CC,CI
 
 # Run the tests in a temporary directory to make sure that we don't import
 # package from the source tree
@@ -45,7 +43,6 @@ deps =
 
     # The devdeps factor is intended to be used to install the latest developer version
     # or nightly wheel of key dependencies.
-    devdeps: git+https://github.com/astropy/astropy.git@main#egg=astropy
     devdeps: git+https://github.com/astropy/specutils.git@main#egg=specutils
 
     cov: pytest-cov
@@ -56,6 +53,7 @@ extras =
     alldeps: all
 
 commands =
+    devdeps: pip install -U -i https://pypi.anaconda.org/astropy/simple astropy --pre
     pip freeze
     !cov: pytest --pyargs synphot {toxinidir}/docs {posargs}
     cov: pytest --pyargs synphot {toxinidir}/docs --cov synphot --cov-config={toxinidir}/setup.cfg {posargs}
@@ -97,6 +95,7 @@ commands =
 changedir = docs
 description = check the links in the HTML docs
 extras = docs
+allowlist_externals = make
 commands =
     pip freeze
     make linkcheck


Test patch:
diff --git a/synphot/tests/test_spectrum_misc.py b/synphot/tests/test_spectrum_misc.py
index 60d2fd7..fc93742 100644
--- a/synphot/tests/test_spectrum_misc.py
+++ b/synphot/tests/test_spectrum_misc.py
@@ -152,6 +152,19 @@ class TestWaveset:
         np.testing.assert_array_equal(bp2.waveset.value, w_true)
         np.testing.assert_array_equal(bp3.waveset.value, w_true)
 
+    def test_box1d_set_step(self):
+        # first test that setting step to default value produces the same
+        # waveset as it did before step was a valid input
+        bp1 = SpectralElement(Box1D, x_0=2000, width=1)
+        assert bp1.waveset.shape == (103,)
+
+        bp2 = SpectralElement(Box1D, x_0=2000, width=1, step=0.01)
+        np.testing.assert_array_equal(bp1.waveset.value, bp2.waveset.value)
+
+        # next test that setting a coarser step produces a smaller waveset
+        bp3 = SpectralElement(Box1D, x_0=2000, width=1, step=0.2)
+        assert bp3.waveset.size < bp1.waveset.size
+
     def test_composite_none(self):
         bp1 = SpectralElement(Box1D, amplitude=1, x_0=5000, width=10)
         bp2 = SpectralElement(Const1D, amplitude=2)