jluastro / PopSyCLE

Population Synthesis for Compact-object Lensing Events
https://popsycle.readthedocs.io/en/latest/
16 stars 3 forks source link

Pbh #15

Closed Kerianne28 closed 4 years ago

Kerianne28 commented 4 years ago

This branch implements everything needed to inject primordial black holes (PBHs) into the .h5 file that will be used by calc_events.

Synthetic.py Changes

_add_pbh:_ This function takes the .h5 file that is output by perform_pop_syn, creates everything needed for the injected PBHs, then combines the PBH data with the data from the previous .h5 file into a new .h5 file.

This function does the following:

_generate_pbh_config_file:_

Note: I am not actually sure if this config file is needed. I followed the layout of other PopSyCLE functions in which some of the parameters that are set when you call the function, are the same as the parameters in the config.yaml file. It may be easier to only have a config file and not put the parameters in the call to the function, or vice versa. For now, I followed the style and created the .yaml file and put the parameters in the call to the function as well.

Run.py changes

Other files added

_pbhconfig.yaml:

_radial_velocity_dispersiondigitized.csv:_

MichaelMedford commented 4 years ago

Additional comments that we just addressed via video chat:

jluastro commented 4 years ago

Actually, the way it is currently implemented is slightly faster. But I would recommend documenting that step a little more.

I also just thought of this: do all hdf5 implementations preserve key order? If not, then Michael’s suggested implementation is preferred or perhaps instead just do:

key_list.remove(‘lat_bin_edges’) key_list.remove(‘lon_bin_edges’)

This will throw an error if they don’t exist; but you can catch it if needed.

On Feb 14, 2020, at 2:12 PM, Michael Medford notifications@github.com wrote:

@MichaelMedford commented on this pull request.

In popsycle/synthetic.py https://github.com/jluastro/PopSyCLE/pull/15#discussion_r379668253:

  • #########
  • Set random seed

  • np.random.seed(seed)
  • t0 = time.time()
  • Define parameters for NFW profile calculations

  • r_s = r_vir/c #kpc, scale radius
  • g = 4.3*(10*-3) #(pckm^2)/(Msun*s^2)
  • h = 70 #km/(s*Mpc)
  • Read in the hdf5 file that doesn't have PBHs. Product of perform_pop_syn.

  • no_pbh_hdf5_file = h5py.File(hdf5_file)
  • key_list = list(no_pbh_hdf5_file)
  • key_list = key_list[:len(key_list)-2] Is this meant to remove the 'lat_bin_edges' and 'lon_bin_edges' keys? If so, then I would rather see something more explicit, like: key_list = [key for key in key_list if 'bin_edges' not in key]

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jluastro/PopSyCLE/pull/15?email_source=notifications&email_token=AALEJQFZ35BL7QCASXHYC4TRC4JMDA5CNFSM4KQDGB22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCVU3JCI#pullrequestreview-359249033, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALEJQGSKC5IGR2MTTX4Q3TRC4JMDANCNFSM4KQDGB2Q.

MichaelMedford commented 4 years ago

I would be surprised if the key order weren't preserved given that the h5py is wrapped around hdf5, that is allocating memory in an order. But I couldn't find documentation on it. My suggestion was motivated by wanting to (1) guarantee correct behavior and (2) improve readability.

On Tue, Feb 18, 2020, 8:39 AM Jessica Lu notifications@github.com wrote:

Actually, the way it is currently implemented is slightly faster. But I would recommend documenting that step a little more.

I also just thought of this: do all hdf5 implementations preserve key order? If not, then Michael’s suggested implementation is preferred or perhaps instead just do:

key_list.remove(‘lat_bin_edges’) key_list.remove(‘lon_bin_edges’)

This will throw an error if they don’t exist; but you can catch it if needed.

On Feb 14, 2020, at 2:12 PM, Michael Medford notifications@github.com wrote:

@MichaelMedford commented on this pull request.

In popsycle/synthetic.py < https://github.com/jluastro/PopSyCLE/pull/15#discussion_r379668253>:

  • #########
  • Set random seed

  • np.random.seed(seed)
  • t0 = time.time()
  • Define parameters for NFW profile calculations

  • r_s = r_vir/c #kpc, scale radius
  • g = 4.3*(10*-3) #(pckm^2)/(Msun*s^2)
  • h = 70 #km/(s*Mpc)
  • Read in the hdf5 file that doesn't have PBHs. Product of

    perform_pop_syn.

  • no_pbh_hdf5_file = h5py.File(hdf5_file)
  • key_list = list(no_pbh_hdf5_file)
  • key_list = key_list[:len(key_list)-2] Is this meant to remove the 'lat_bin_edges' and 'lon_bin_edges' keys? If so, then I would rather see something more explicit, like: key_list = [key for key in key_list if 'bin_edges' not in key]

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/jluastro/PopSyCLE/pull/15?email_source=notifications&email_token=AALEJQFZ35BL7QCASXHYC4TRC4JMDA5CNFSM4KQDGB22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCVU3JCI#pullrequestreview-359249033>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AALEJQGSKC5IGR2MTTX4Q3TRC4JMDANCNFSM4KQDGB2Q .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jluastro/PopSyCLE/pull/15?email_source=notifications&email_token=AC3R7JEG7MKI6FKWSN7ET5LRDQFMBA5CNFSM4KQDGB22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMCV4WY#issuecomment-587554395, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC3R7JG5SLG6MSYEU54A2I3RDQFMBANCNFSM4KQDGB2Q .