Refactor SN Ia e2e test to use oversampled opsim #132

hombit · 2024-09-26T15:57:02Z

This is a part of work to incorporate noise generation into the simulation pipeline started in #108

The changes:

Introduces oversampled_observations fixture
Changes the interface of test_snia_end2end and helper test function draw_single_random_sn to use opsim only, removes phases calculations
This new changes revealed some inconsistency between band fluxes we produce and sncosmo produces. I changed passbands we use from ur to ri and model from SALT2 to SALT3. I opened an issue about this inconsistency Investigate inconsistency between sncosmo and our band fluxes #133
I fix some broken NB code and modified NB to use the new test interface. I opened issue Run notebooks in CI #131 to run notebooks on CI.
Since NB called the test functions (I believe it was not the best idea), it also needs to access some pytest fixtures. I introduced an ugly hack in conftest.py to fix it. I hope someone would volunteer to fix it in a proper way.

Change Description

My PR includes a link to the issue that I am addressing

Solution Description

Code Quality

I have read the Contribution Guide
My code follows the code style of this project
My code builds (or compiles) cleanly without any errors or warnings
My code contains relevant comments and necessary documentation

Project-Specific Pull Request Checklists

Bug Fix Checklist

My fix includes a new test that breaks as a result of the bug (if possible)
My change includes a breaking change
- My change includes backwards compatibility and deprecation warnings (if possible)

New Feature Checklist

I have added or updated the docstrings associated with my feature using the NumPy docstring format
I have updated the tutorial to highlight my new feature (if appropriate)
I have added unit/End-to-End (E2E) test cases to cover my new feature
My change includes a breaking change
- My change includes backwards compatibility and deprecation warnings (if possible)

Documentation Change Checklist

Any updated docstrings use the NumPy docstring format

Build/CI Change Checklist

If required or optional dependencies have changed (including version numbers), I have updated the README to reflect this
If this is a new CI setup, I have added the associated badge to the README

Other Change Checklist

Any new or updated docstrings use the NumPy docstring format.
I have updated the tutorial to highlight my new feature (if appropriate)
I have added unit/End-to-End (E2E) test cases to cover any changes
My change includes a breaking change
- My change includes backwards compatibility and deprecation warnings (if possible)

# Conflicts: # tests/tdastro/conftest.py

review-notebook-app · 2024-09-26T15:57:08Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

tests/tdastro/sources/test_snia.py

jeremykubica · 2024-09-26T18:10:12Z

docs/notebooks/test_snia.ipynb

-    "res = test_snia_end2end(\n",
-    "    None, opsim_db_file=None, opsim=False, nsample=10, return_result=True, phase_rest=np.linspace(-15, 45, 20)\n",
+    "res, passbands = test_snia_end2end(\n",
+    "    conftest.oversampled_observations(conftest.opsim_shorten(conftest.test_data_dir())),\n",


There are a few different approaches we could use to access the opsim_shorten here without having to change the pytest fixtures in the conftest.py file.

[My preference] Move the directory names to the top level __init__.py file (or a new paths.py file in the base directory):

_TDASTRO_BASE_DIR = os.path.dirname(__file__) _TDASTRO_TEST_DIR = os.path.join(_TDASTRO_BASE_DIR, "tests", "tdastro") _TDASTRO_TEST_DATA_DIR = os.path.join(_TDASTRO_TEST_DIR, "data")

Then both the notebooks and conftest.py use these.

We would need to include a cell to do:

base_opsim = OpSim.from_db(opsim_shorten) oversampled_observations = oversample_opsim( base_opsim, pointing=(0.0, 0.0), search_radius=180.0, delta_t=0.01, time_range=(61406.0, 61771.0), bands=None, strategy="darkest_sky", )

but I think it would be good to explicitly include that in the notebook. It allows the user to see where you are pulling the data and how you are transforming it.

[Not prefered] I think in notebooks it is fair to just provide a path like

test_opsim_filename = "../../tests/tdastro/data/opsim_shorten.db")

It's not as robust as the approach above, but that is okay since users are expected to interact with the notebooks.

jeremykubica · 2024-09-26T18:18:40Z

docs/notebooks/test_snia.ipynb

   "outputs": [],
   "source": [
-    "res = test_snia_end2end(\n",
-    "    None, opsim_db_file=None, opsim=False, nsample=10, return_result=True, phase_rest=np.linspace(-15, 45, 20)\n",
+    "res, passbands = test_snia_end2end(\n",


My preference would be not to use test_snia_end2end here and instead repeat the code in the notebook. I know code repetition is usually considered bad, but I think the code of interest to the notebook user will be what is in that function. Leaving the code out of the notebook doesn't provide the user all the information.

Alternatively, if we really want to keep code in sync, we could break out modules into new files.

We have planned to update notebook to show how model is defined with more text descriptions by breaking the end2end test into multiple steps. Right now I prefer to keep it this way so I don't need to change the same thing in two different places when experimenting.

I agree with this, but I've just keep the same NB structure as it was before. I'd keep these changes out of the scope of this PR or ask someone to help me commiting/PRing these changes

jeremykubica · 2024-09-26T18:18:53Z

tests/tdastro/sources/test_snia.py

@@ -174,22 +154,22 @@ def test_snia_end2end(
        saltpars = {"x0": p["x0"], "x1": p["x1"], "c": p["c"], "z": p["redshift"], "t0": p["t0"]}
        model = sncosmo.Model(sncosmo_modelname)


I'd like to find an alternate way to test the code. sncosmo.Model will download the model files each time the test is run in a new environment (including each run of the CI tests). This is already causing intermittent failures on github. We should isolate the tests from external downloads. We could do this by pre-downloading and including the model files or by comparing with known data.

jeremykubica · 2024-09-26T18:21:05Z

tests/tdastro/conftest.py

@@ -6,37 +6,69 @@
 TEST_DIR = os.path.dirname(__file__)


-@pytest.fixture
+def fixture(func):


I would prefer to avoid this by either 1) moving the directory information into the base directory or 2) adding the full path to the notebook. See the discussion in the notebook section.

Removes the conftest changes and uses a few manual strings in the notebook instead.

jeremykubica

I made the suggested changes. Can you review and, if happy, merge?

hombit added 4 commits September 26, 2024 11:43

overampled_observations fixture

868404a

# Conflicts: # tests/tdastro/conftest.py

Change oversampled_observations to cover a year

6c557c6

Use oversampled opsim in Ia test

2fba726

Refactor test_snia_end2end with oversampled opsim

dcaf0da

hombit requested review from mi-dai and jeremykubica September 26, 2024 15:57

mi-dai requested changes Sep 26, 2024

View reviewed changes

tests/tdastro/sources/test_snia.py Outdated Show resolved Hide resolved

tests/tdastro/sources/test_snia.py Show resolved Hide resolved

tests/tdastro/sources/test_snia.py Show resolved Hide resolved

snia e2e: Restore original redshift range

35a0f3c

hombit requested a review from mi-dai September 26, 2024 18:19

jeremykubica requested changes Sep 26, 2024

View reviewed changes

mi-dai approved these changes Sep 26, 2024

View reviewed changes

jeremykubica added 2 commits September 27, 2024 08:46

Make suggested changes to the PR

39fea63

Removes the conftest changes and uses a few manual strings in the notebook instead.

Clear notebook output

a8013a5

jeremykubica approved these changes Sep 27, 2024

View reviewed changes

hombit merged commit ad925bc into main Sep 27, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor SN Ia e2e test to use oversampled opsim #132

Refactor SN Ia e2e test to use oversampled opsim #132

hombit commented Sep 26, 2024 •

edited

Loading

review-notebook-app bot commented Sep 26, 2024

jeremykubica Sep 26, 2024

jeremykubica Sep 26, 2024

mi-dai Sep 26, 2024

hombit Sep 26, 2024

jeremykubica Sep 26, 2024

jeremykubica Sep 26, 2024

jeremykubica left a comment

		@@ -174,22 +154,22 @@ def test_snia_end2end(
		saltpars = {"x0": p["x0"], "x1": p["x1"], "c": p["c"], "z": p["redshift"], "t0": p["t0"]}
		model = sncosmo.Model(sncosmo_modelname)

Refactor SN Ia e2e test to use oversampled opsim #132

Refactor SN Ia e2e test to use oversampled opsim #132

Conversation

hombit commented Sep 26, 2024 • edited Loading

Change Description

Solution Description

Code Quality

Project-Specific Pull Request Checklists

Bug Fix Checklist

New Feature Checklist

Documentation Change Checklist

Build/CI Change Checklist

Other Change Checklist

review-notebook-app bot commented Sep 26, 2024

jeremykubica Sep 26, 2024

Choose a reason for hiding this comment

jeremykubica Sep 26, 2024

Choose a reason for hiding this comment

mi-dai Sep 26, 2024

Choose a reason for hiding this comment

hombit Sep 26, 2024

Choose a reason for hiding this comment

jeremykubica Sep 26, 2024

Choose a reason for hiding this comment

jeremykubica Sep 26, 2024

Choose a reason for hiding this comment

jeremykubica left a comment

Choose a reason for hiding this comment

hombit commented Sep 26, 2024 •

edited

Loading