Add redshift effect #16

OliviaLynn · 2024-07-02T20:35:56Z

Change Description

Closes #4

Solution Description

Redshift conversions

Converts desired observation_times, observation_wavelengths to emitted_times, emitted_wavelengths.
We then feed these emitted_times, emitted_wavelengths values to a given PhysicalModel to get the emitted_flux.
Finally, we convert the emitted_flux to observed_flux to get the flux densities as they would appear on Earth.

Conversions used are:
- emitted_wavelength = observed_wavelength / (1 + redshift)
- emitted_time = observed_time / (1 + redshift)
- observed_flux = emitted_flux / (1 + redshift)

Pre-effects

Note that this PR also creates the concept of "pre-effect," which we apply to times, wavelengths in the PhysicalModel's evaluate, just before we call flux_density = self._evaluate(times, wavelengths, **kwargs).

Our initial thoughts were that the only other effect that might implement a pre_effect method would be some kind of lensing (I think Mi said strong lensing?); if we do add another such pre_effect, we should consider the ordering and other factors that could be involved in them playing nicely together.

src/tdastro/effects/redshift.py

jeremykubica

Looks really good. I will let the scientists do the actual review to check the correctness of the computations. I added a few small comments.

src/tdastro/effects/redshift.py

…en adding

OliviaLynn · 2024-07-09T05:25:44Z

Questions for tomorrow:
~~1. State of "required parameters"/is it ok to have redshift as a required param~~
~~2. Would like to go over pre-effect docstring, feels wordy and I don't know standard language in this area~~
~~3. Would like us to further discuss epochs/how we line up timing as part of the shift-and-scale adjustment that is our redshift calculation~~

src/tdastro/effects/redshift.py

OliviaLynn · 2024-07-10T20:02:34Z

Update July 10

Changes:

Main: Time calculations now performed with respect to epoch aka t0
Converted the EffectModel's required_parameters from a method to an attribute
Uploaded a redshift-demo notebook to a branch in tdastro-sandbox

Questions:

Have some outstanding questions about nomenclature/general wording - especially in 3 places, in links just below, but also highlighted in the PR with a comment per line-of-interest
a. L27: General wording; also, are we picking t0 or epoch?
a. L41: I use "observed" and "emitted" throughout the pre-effect code. Should I use something closer to "observation frame" and "rest frame"? (Is rest frame even applicable here?)
a. L58: Someone tied to previous. I'm not sure how to word what's going on here in a way that's readable but still provides all the information needed.

src/tdastro/effects/redshift.py

mi-dai · 2024-07-10T20:59:07Z

src/tdastro/effects/redshift.py

+        emitted_times_rel_to_t0 = observed_times_rel_to_t0 / (1 + self.redshift)
+        emitted_times = emitted_times_rel_to_t0 + self.t0
+        emitted_wavelengths = observed_wavelengths / (1 + self.redshift)
+        return (emitted_times, emitted_wavelengths)


I think what the model usually provide in its look up table is actually this emitted_times_rel_to_t0, not emitted_times, so we should return that in terms of looking up for the flux_density.

Since we'd like to have a uniform interface, I'd vote to all models provide the same evaluate(times, wavelengths), where times are not relative to t0. Since model knows its t0, it is not a big deal to use it to get relative times, while computational overhead is negligible (and potentially zero with JIT)

OK. I'm fine with using times instead of relative times. I would still like to see if this is convenient enough for users after implementing some models.

src/tdastro/base_models.py

jeremykubica · 2024-07-11T11:40:56Z

src/tdastro/effects/redshift.py

+        super().__init__(**kwargs)
+        self.required_parameters = ["redshift", "t0"]
+        self.add_parameter("redshift", redshift, required=True, **kwargs)
+        self.add_parameter("t0", t0, required=True, **kwargs)


I hadn't thought about doing things this way, but this might be preferable.

The way the code is currently structured these are not meant to be included as parameters for the effect model, but rather to pull them from the physical model (via required_parameters). So for applying an effect, we would do something like: physical_model.redshift.

But this approach could be cleaner. We could get rid of required_parameters altogether and just pass in the PhysicalModel the way you do in the test scripts.

I need to think through the pros and cons a bit.

I believe that redshift and t0 are physical model's parameters, not effect's parameters

jeremykubica · 2024-07-11T11:41:51Z

src/tdastro/effects/redshift.py

+        flux_density : `numpy.ndarray`
+            The redshifted results.
+        """
+        return flux_density / (1 + self.redshift)


See the overall comment in the __init__() function about overall approach.

In the current set up, this should be physical_model.redshift since we want to use a specific redshift for each object.

For this and my comments throughout: Olivia and I chatted about it today. If we make effects behave like other nodes in the graph, they will have consistent sampling behavior. In that case we will want to have the effect take in the PhysicalModel's parameters.

The pros of this approach is that 1) it keeps everything in the same graph formulation, 2) keeps the same sampling counter throughout the entire graph, 3) doesn't require a specialized interface for effects (the required_parameters list), and 4) allows for stateful effects.

The cons are: 1) as @hombit) notes there is a chance this could be misspecified during model creation, 2) it requires creating a unique effect for each physical model.

Let's go with the current approach for now. If this causes too much confusion, we can change back to stateless effects.

jeremykubica · 2024-07-11T11:43:13Z

src/tdastro/effects/redshift.py

+            which will then be redshifted to observation-frame flux densities at the observation-frame
+            times and wavelengths. # TODO WORDING (3/3)
+        """
+        observed_times_rel_to_t0 = observed_times - self.t0


See the overall comment in the __init__() function about overall approach.

In the current set up, the t0 and redshift parameters should come from the PhysicalModel applying this effect instead of the effect's parameter. So you will need to pass a PhysicalModel into pre_effect().

jeremykubica · 2024-07-11T11:44:35Z

src/tdastro/base_models.py

@@ -241,9 +243,15 @@ def evaluate(self, times, wavelengths, resample_parameters=False, **kwargs):
        if resample_parameters:
            self.sample_parameters(kwargs)

+        for effect in self.effects:
+            if hasattr(effect, "pre_effect"):  # pre-effects == adjustments done to times and/or wavelengths
+                times, wavelengths = effect.pre_effect(times, wavelengths, **kwargs)


I think we will want to pass a reference to the PhysicalModel into pre_effect. See the comments in redshift.py. So this would be:

times, wavelengths = effect.pre_effect(times, wavelengths, physical_model=self, **kwargs)

Or something similar.

jeremykubica · 2024-07-11T11:46:17Z

tests/tdastro/effects/test_redshift.py

+    """Get the values for a source with no effects and a redshifted source."""
+    model_no_effects = StepSource(brightness=brightness, t0=t0, t1=t1)
+    model_redshift = StepSource(brightness=brightness, t0=t0, t1=t1, redshift=redshift)
+    model_redshift.add_effect(Redshift(redshift=model_redshift, t0=model_redshift))


See the overall comment in the __init__() function about overall approach.

If we stick with the current approach, we could just make this: model_redshift.add_effect(Redshift())

However I'm thinking your approach might be cleaner.

I agree with Jeremy, we shouldn't pass redshifts and t0s twice to both classes, even implicitly. We might see bad bugs with these approach when parameters are random number generators...

src/tdastro/base_models.py

src/tdastro/effects/redshift.py

hombit · 2024-07-11T14:44:56Z

src/tdastro/effects/redshift.py

+        super().__init__(**kwargs)
+        self.required_parameters = ["redshift", "t0"]
+        self.add_parameter("redshift", redshift, required=True, **kwargs)
+        self.add_parameter("t0", t0, required=True, **kwargs)


I believe that redshift and t0 are physical model's parameters, not effect's parameters

src/tdastro/effects/redshift.py

hombit · 2024-07-11T14:49:16Z

src/tdastro/effects/redshift.py

+        emitted_times_rel_to_t0 = observed_times_rel_to_t0 / (1 + self.redshift)
+        emitted_times = emitted_times_rel_to_t0 + self.t0
+        emitted_wavelengths = observed_wavelengths / (1 + self.redshift)
+        return (emitted_times, emitted_wavelengths)


Since we'd like to have a uniform interface, I'd vote to all models provide the same evaluate(times, wavelengths), where times are not relative to t0. Since model knows its t0, it is not a big deal to use it to get relative times, while computational overhead is negligible (and potentially zero with JIT)

hombit · 2024-07-11T14:51:10Z

tests/tdastro/effects/test_redshift.py

+    """Get the values for a source with no effects and a redshifted source."""
+    model_no_effects = StepSource(brightness=brightness, t0=t0, t1=t1)
+    model_redshift = StepSource(brightness=brightness, t0=t0, t1=t1, redshift=redshift)
+    model_redshift.add_effect(Redshift(redshift=model_redshift, t0=model_redshift))


I agree with Jeremy, we shouldn't pass redshifts and t0s twice to both classes, even implicitly. We might see bad bugs with these approach when parameters are random number generators...

tests/tdastro/effects/test_redshift.py

…arameter with required=True

jeremykubica · 2024-07-17T00:03:28Z

src/tdastro/effects/redshift.py

+        flux_density : `numpy.ndarray`
+            The redshifted results.
+        """
+        return flux_density / (1 + self.redshift)


For this and my comments throughout: Olivia and I chatted about it today. If we make effects behave like other nodes in the graph, they will have consistent sampling behavior. In that case we will want to have the effect take in the PhysicalModel's parameters.

The pros of this approach is that 1) it keeps everything in the same graph formulation, 2) keeps the same sampling counter throughout the entire graph, 3) doesn't require a specialized interface for effects (the required_parameters list), and 4) allows for stateful effects.

The cons are: 1) as @hombit) notes there is a chance this could be misspecified during model creation, 2) it requires creating a unique effect for each physical model.

Let's go with the current approach for now. If this causes too much confusion, we can change back to stateless effects.

Add redshift effect

2f31b84

OliviaLynn requested review from hombit, aimalz and mi-dai July 2, 2024 20:53

OliviaLynn marked this pull request as ready for review July 2, 2024 20:53

hombit reviewed Jul 2, 2024

View reviewed changes

src/tdastro/effects/redshift.py Outdated Show resolved Hide resolved

hombit reviewed Jul 3, 2024

View reviewed changes

src/tdastro/effects/redshift.py Outdated Show resolved Hide resolved

mi-dai reviewed Jul 3, 2024

View reviewed changes

src/tdastro/effects/redshift.py Outdated Show resolved Hide resolved

Update pz to redshift

150f45c

jeremykubica reviewed Jul 8, 2024

View reviewed changes

src/tdastro/effects/redshift.py Outdated Show resolved Hide resolved

src/tdastro/effects/redshift.py Outdated Show resolved Hide resolved

src/tdastro/effects/redshift.py Outdated Show resolved Hide resolved

Add PR comments, plus the beginning of required parameter code I'd be…

6284779

…en adding

OliviaLynn marked this pull request as draft July 9, 2024 05:21

OliviaLynn added 4 commits July 10, 2024 11:09

Merge remote-tracking branch 'origin/main' into redshift

fadb03b

Update redshift to be relative to t0 and take params from given model

ea8ef0a

Convert required_parameters to attribute; tidy up redshift's apply

67413bc

Update unit tests to use new time-interval calculation

2148ce7

OliviaLynn commented Jul 10, 2024

View reviewed changes

src/tdastro/effects/redshift.py Outdated Show resolved Hide resolved

OliviaLynn commented Jul 10, 2024

View reviewed changes

src/tdastro/effects/redshift.py Outdated Show resolved Hide resolved

OliviaLynn commented Jul 10, 2024

View reviewed changes

src/tdastro/effects/redshift.py Show resolved Hide resolved

OliviaLynn marked this pull request as ready for review July 10, 2024 20:06

OliviaLynn requested review from hombit, mi-dai and jeremykubica July 10, 2024 20:07

mi-dai reviewed Jul 10, 2024

View reviewed changes

jeremykubica reviewed Jul 11, 2024

View reviewed changes

hombit reviewed Jul 11, 2024

View reviewed changes

OliviaLynn added 2 commits July 16, 2024 00:56

Incorporate comments on nomenclature; fix docstring equation

761b57e

Merge branch 'main' into redshift

64098cd

OliviaLynn added 3 commits July 16, 2024 14:35

Add back pre-effects to moved PhysicalModel

24d5f56

Remove EffectModel's required_parameters, as new strategy calls add_p…

b982a95

…arameter with required=True

Update import path

580ca5f

OliviaLynn requested a review from jeremykubica July 16, 2024 19:27

OliviaLynn mentioned this pull request Jul 16, 2024

Redshift: Add unit test checking that wavelengths are redshifted #36

Open

jeremykubica approved these changes Jul 17, 2024

View reviewed changes

OliviaLynn merged commit a94c158 into main Jul 17, 2024
4 checks passed

OliviaLynn deleted the redshift branch August 9, 2024 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add redshift effect #16

Add redshift effect #16

OliviaLynn commented Jul 2, 2024 •

edited

Loading

jeremykubica left a comment

OliviaLynn commented Jul 9, 2024 •

edited

Loading

OliviaLynn commented Jul 10, 2024

mi-dai Jul 10, 2024 •

edited

Loading

hombit Jul 11, 2024

mi-dai Jul 11, 2024

jeremykubica Jul 11, 2024

hombit Jul 11, 2024

jeremykubica Jul 11, 2024

jeremykubica Jul 17, 2024

jeremykubica Jul 11, 2024

jeremykubica Jul 11, 2024

jeremykubica Jul 11, 2024

hombit Jul 11, 2024

hombit Jul 11, 2024

hombit Jul 11, 2024

hombit Jul 11, 2024

jeremykubica Jul 17, 2024

Add redshift effect #16

Add redshift effect #16

Conversation

OliviaLynn commented Jul 2, 2024 • edited Loading

Change Description

Solution Description

Redshift conversions

Pre-effects

jeremykubica left a comment

Choose a reason for hiding this comment

OliviaLynn commented Jul 9, 2024 • edited Loading

OliviaLynn commented Jul 10, 2024

Update July 10

Changes:

Questions:

mi-dai Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliviaLynn commented Jul 2, 2024 •

edited

Loading

OliviaLynn commented Jul 9, 2024 •

edited

Loading

mi-dai Jul 10, 2024 •

edited

Loading