21 cm part updated #128

Anikbh11 · 2020-03-27T16:39:35Z

Hello,

Apologies for the delays.

Following up on Britton's suggestion, I've added an entirely new function to calculate the tau_profile for the 21 cm line (https://arxiv.org/abs/1510.02296 eq 1). From what I tested on my dataset (Gadget Owls) this feature works fine in both velocity and wavelength space.

I just saw the length of ray fix that Britton merged yesterday, that is great news for the 21 cm part too. At the moment I ignore the peculiar velocity gradients, I'll include complete treatment of peculiar velocities in the 21 cm forest in a following update (this needs the 'l' values ).

Thanks,
Aniket

brittonsmith

Hi @Anikbh11, thanks for reissuing your PR here. This would be a great feature to have. There is a few additional things we'll need before merging this:

full narrative documentation (in doc/source)
some tests of this functionality that can be run as part of the test suite

Additionally, can you confirm that this is now working as expected and perhaps post a sample image of the final product? Finally, the testing infrastructure has flagged a handful of style issues, like trailing whitespace and unused imports. The easiest way to take care of these is to install flake8 with pip and then run flake8 trident from the top directory of the repo.

I'm happy to work with you on the other changes.

trident/absorption_spectrum/absorption_line.py

trident/absorption_spectrum/absorption_spectrum.py

trident/absorption_spectrum/absorption_line.py

trident/absorption_spectrum/absorption_spectrum.py

too

brittonsmith

@Anikbh11, thanks for these updates. I've left a few comments regarding the overall structure of this. I'll save comments on minor formatting issues until after we've converged on the larger issues.

brittonsmith · 2020-04-21T12:37:31Z

trident/absorption_spectrum/absorption_line.py

+    if deposition_method == 'voigt':
+        phi = voigt(a, x)                               # line profile
+    else:
+        phi = delta(lam1,lambda_bins)
    tauphi = tau0 * phi              # profile scaled with tau0

    return (lambda_bins, tauphi)

 def tau_profile_21cm(lambda_0, f_value, gamma, temperature, number_density,


I don't think we need both a tau_profile_21cm function and a variable deposition method in the regular tau_profile function. I would say choose one or the other, perhaps whichever option has the least amount of duplicate code.

Agreed. I would say we could just do with tau_profile and have a keyword which triggers the 21 cm deposition to be different, and in turn, this would get triggered by using a line like: H I 21cm or something?

brittonsmith · 2020-04-21T12:44:06Z

trident/absorption_spectrum/absorption_spectrum.py

@@ -897,18 +903,20 @@ def _add_lines_to_spectrum(self, field_data, use_peculiar_velocity,
                             'increasing the bin size.') % my_vbins.size)

                    # the virtual bins and their corresponding opacities
-                    if (line['wavelength'].d == 2.1e9): 
+                    if (line['label'] == '21 cm'): 


Similar to my comment above, it's somewhat redundant to be able to switch the deposition method and change the behavior for one specific line. Because of the huge range of wavelengths between the hyperfine transition and the electron energy level transitions, I don't think anyone should be making a spectrum that includes both the 21 cm and something from the Lyman series. I'd rather see a workflow where the user specifies that they want only the 21 cm line and the delta deposition method manually. This would remove the need to have separate function calls here, which is going to be more difficult to maintain and sets a precedent for adding even more.

Agree with Britton.

brittonsmith · 2020-04-21T12:45:21Z

trident/data/line_lists/lines.txt

@@ -1,5 +1,6 @@
 #Ion        Wavelength [A]           gamma         f_value       alt. name
 H I            1215.670000    4.690000e+08    4.160000e-01            Ly a
+H I            2.1000000e9    2.850000e-15    4.160000e-01            21 cm


This is quite pedantic, but the rest of the H lines are in order of decreasing wavelength. I would put this line above Lya.

brittonsmith · 2020-04-21T12:45:45Z

trident/data/line_lists/lines.txt

@@ -49,7 +50,7 @@ C I            1193.031000    6.390000e+07    4.090000e-02
 C I            1188.833000    1.950000e+07    1.240000e-02                
 C I            1157.910000    3.520000e+07    2.120000e-02                
 C II           1335.663000    4.760000e+07    1.270000e-02      C II* 1336
-C II           1334.532000    2.410000e+08    1.290000e-01                
+C II           1334.532000    2.400000e+08    1.280000e-01                


@chummels, can you comment on these changes to the line data?

These changes are in line with the Morton data, but it just seems odd to include these minor changes in a PR that's about depositing HI as 21 cm lines. But they can stay, as I don't think they'll break the answer tests. I don't think so anyway.

chummels

In general, this is really cool functionality that you've added. I agree with the comments that @brittonsmith listed with a few additional mentions below. I think you'll want to include a description of this somewhere in the documentation, maybe with a brief example. And make a test for it. But overall, I like it!

chummels · 2020-06-18T02:32:04Z

trident/absorption_spectrum/absorption_line.py

@@ -213,11 +231,101 @@ def tau_profile(lambda_0, f_value, gamma, v_doppler, column_density,
    # tau_0
    tau_X = tau_factor * column_density * f_value / v_doppler
    tau0 = tau_X * lambda_0 * 1e-8
-
+    


I think whitespace changes like these will fail the flake tests. In general, try to leave whitespace alone.

chummels · 2020-06-18T02:33:13Z

trident/absorption_spectrum/absorption_spectrum.py

    """

    def __init__(self, lambda_min, lambda_max, n_lambda=None, dlambda=None,
-                 bin_space='wavelength'):
+                 bin_space='wavelength',deposition_method='voigt'):


Minor issue, but separate keyword arguments by a space.

chummels · 2020-06-18T02:46:20Z

trident/absorption_spectrum/absorption_spectrum.py

@@ -468,8 +482,10 @@ def make_spectrum(self, input_object, output_file=None,
        input_ds.domain_left_edge = input_ds.domain_left_edge.to('code_length')
        input_ds.domain_right_edge = input_ds.domain_right_edge.to('code_length')

-        if self.bin_space == 'velocity':
-            self.zero_redshift = getattr(input_ds, 'current_redshift', 0)
+        self.hubble_constant = getattr(input_ds,'hubble_constant')


I fear that this may break functionality for simulations which aren't cosmological. Notice for the redshift, there is a final argument that sets it to 0 in the case that there is no current_redshift parameter. This is to ensure that the code works on non-cosmological datasets by just setting the current redshift value to 0.

Could you group these parameter instantiations along with your co = Cosmology and hnow code below, and maybe put it inside an if/then block for deposition_method='delta', or a try/except block and give a default value for the z=0 case, so non-cosmological datasets will still run?

chummels · 2020-06-18T02:46:49Z

trident/absorption_spectrum/absorption_spectrum.py

@@ -738,19 +755,23 @@ def _add_lines_to_spectrum(self, field_data, use_peculiar_velocity,

            # the total number of absorbers per transition
            n_absorbers = len(lambda_obs)
+            temperature = field_data['temperature'].d


Is this necessary to strip off the Kelvin unit?

chummels · 2020-06-18T02:48:17Z

trident/absorption_spectrum/absorption_spectrum.py

@@ -468,8 +482,10 @@ def make_spectrum(self, input_object, output_file=None,
        input_ds.domain_left_edge = input_ds.domain_left_edge.to('code_length')
        input_ds.domain_right_edge = input_ds.domain_right_edge.to('code_length')

-        if self.bin_space == 'velocity':


So we won't be able to do in velocity space? Seems like leaving this in would allow for it, but maybe not?

21 cm part updated

5b04665

brittonsmith reviewed Mar 30, 2020

View reviewed changes

Anikbh11 added 2 commits April 9, 2020 16:01

Added a new option to select line deposition method

c75c6e3

Minor update ensuring lightray redshift is global for wavelength space

4edad87

too

brittonsmith reviewed Apr 21, 2020

View reviewed changes

chummels reviewed Jun 18, 2020

View reviewed changes

chummels mentioned this pull request Jun 26, 2020

HI absorption in simulations with radiative transfer #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

21 cm part updated #128

21 cm part updated #128

Anikbh11 commented Mar 27, 2020

brittonsmith left a comment

brittonsmith left a comment

brittonsmith Apr 21, 2020

chummels Jun 18, 2020

brittonsmith Apr 21, 2020

chummels Jun 18, 2020

brittonsmith Apr 21, 2020

brittonsmith Apr 21, 2020

chummels Jun 18, 2020

chummels left a comment

chummels Jun 18, 2020

chummels Jun 18, 2020

chummels Jun 18, 2020

chummels Jun 18, 2020

chummels Jun 18, 2020

21 cm part updated #128

Are you sure you want to change the base?

21 cm part updated #128

Conversation

Anikbh11 commented Mar 27, 2020

brittonsmith left a comment

Choose a reason for hiding this comment

brittonsmith left a 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

chummels left a 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