removing all hard coded values

[fedhere]

hi yuqian, there are still a lot of hard coded things in the code. for example: you had a line where you were removing the last value of the inpyt wavelength array np.array(s2['wavelog_input'][:1024])

with the hard coded 1024 size.

i changed it to

np.array(s2['wavelog_input'][:y_flat.size]) so that it only happens if the files are of different size, but it does not if the user uses the correct input sizes.

several wavelength ranges specific to the Fe line were hard coded in. Please go through the code and make sure all the appropriate values are set to dynamic variables, so that the code can be used by others.

[yoyo1989]

Thanks Fed! I went through the code and didn't see hard coded values.

[mmoJazz]

OK so is this closed? I'd like to get a sense of where we stand ..

Thanks

[yoyo1989]

It's not closed. Although I went through the code and didn't see hard coded values, I'm afraid that there are still some hard coded values, so I didn't close this issue.

[fedhere]

lets give it one more look but we have to close this, and all other issues before going public.

[mmoJazz]

• Actually there is still hardcoded values FFT_smooth.pro, starting line 46: "; filtering spectra
  num_upper=max(where(1.0/freq[1:num_bin-1] * 3.0e5 * binsize GT cut_vel)) num_lower=max(where(1.0/freq[1:num_bin-1] * 3.0e5 * binsize GT 1.0e5, num_num_lower))"

1) so there is 1D5 - I presume it stands for 100,000 km/s the velocity we say in Yuqian's appendix is the velocity value above which we don't use the Powerspectrum magnitude to fit for (ie "ie the first 5 wavenumbers). So we need to declare that variable at the top - for example: vel_toolargeforSN_kms=1.D5 ; Velocity limit for whose corresponding wavenumbers are not included in the fit as they are too large to be associated with SN features

2) I presume 3E5 stands for the speed of light? If so, then a) we need to be explicit about it b) it's actually not exactly 300,000 km/s but 299792.47 km/s. The rounding probably won't matter here, but it's good practice to import/define basic physical constants at the top (e.g. I have an IDL file with constants that I use and their exact values that I import at the beginning, so I don't have to always look up the exact value)

• as to cut_vel: shall we give people the number we have generally been using for our work? I think that's 1000 km/s which we're mentioning in Yuqina's appendix

[fedhere]

On my hand: the hard coded values were taken care of in the python code. The IDL code I did not look

On Thu, Jul 14, 2016 at 11:46 AM, mmoJazz notifications@github.com wrote:

• Actually there is still hardcoded values FFT_smooth.pro, starting line 46: "; filtering spectra num_upper=max(where(1.0/freq[1:num_bin-1]_3.0e5_binsize GT cut_vel)) num_lower=max(where(1.0/freq[1:num_bin-1]_3.0e5_binsize GT 1.0e5, num_num_lower))"

1) so there is 1D5 - I presume it stands for 100,000 km/s the velocity we say in Yuqian's appendix is the velocity value above which we don't use the Powerspectrum magnitude to fit for (ie "ie the first 5 wavenumbers). So we need to declare that variable at the top - for example: vel_toolargeforSN_kms=1.D5 ; Velocity limit for whose corresponding wavenumbers are not included in the fit as they are too large to be associated with SN features

2) I presume 3E5 stands for the speed of light? If so, then a) we need to be explicit about it b) it's actually not exactly 300,000 km/s but 299792.47 km/s. The rounding probably won't matter here, but it's good practice to import/define basic physical constants at the top (e.g. I have an IDL file with constants that I use and their exact values that I import at the beginning, so I don't have to always look up the exact value)

• as to cut_vel: shall we give people the number we have generally been using for our work? I think that's 1000 km/s which we're mentioning in Yuqina's appendix

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nyusngroup/SESNspectraLib/issues/13#issuecomment-232705395, or mute the thread https://github.com/notifications/unsubscribe/ABnkhqQ2Y1MobVD61ItNbFsCr8xGUGAiks5qVlnjgaJpZM4JIQ_z .

/|/\ |* Please consider the environment before printing this e-mail

[mmoJazz]

Fed: sure, but I'm just soliciting also your thoughts on this, even if it's in IDL :) THanks!

[fedhere]

i dont know the details, but in general i second that it is good to have constants defined at the top, so they can be easily changed, and physical constants to be accurate (there are library that define them in most languages.

I am no longer fluent in idl, so while I am happy to give my 2 cents I dont think i can browse the code looking for hard coded values

On Thu, Jul 14, 2016 at 3:51 PM, mmoJazz notifications@github.com wrote:

Fed: sure, but I'm just soliciting also your thoughts on this, even if it's in IDL :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nyusngroup/SESNspectraLib/issues/13#issuecomment-232772817, or mute the thread https://github.com/notifications/unsubscribe/ABnkhogOEPzC3G7FZgJ7frABNcYQiherks5qVpM3gaJpZM4JIQ_z .

/|/\ |* Please consider the environment before printing this e-mail

[yoyo1989]

Yes, 1.0e5 stands for 100,000 km/s in the appendix and 3e5 stands for the speed of light. I agree with you. I'll define 1.0e5 and speed of light at the top of the code. I'll mention the 1000km/s at the top of the code as well.

[mmoJazz]

I editted SNspecFFTsmooth.pro to remove hard-coded values and also change the name of the code inside the code! Note that the velocity separating noise from signal will be between cut_vel (the smallest velocity we choose by hand, 1000 km/s) and vel_toolargeforSN (100,000 km/s) by construction. Actually in hindsight, cut_vel should be called, for consistency, vel_toosmallforSN - and those are 2 choices we're making and that should be clear to the reader.

@yoyo1989 can you please look at the code and re-run it? Anything else that needs to be done to close this issue?

[yoyo1989]

I've looked at the code and re-run it. Everything looks good.