GBTAmmoniaSurvey / GAS

observing scripts and files related to the GBT Ammonia Survey (GAS, PI: Jaime E Pineda & Rachel Friesen)
MIT License
6 stars 13 forks source link

Gaussian fitting function for non-NH3 lines #160

Closed jakeown closed 7 years ago

rfriesen commented 7 years ago

Hey @jakeown, this looks great. I was able to run it on B18 C2S with no problems. Would it be possible to update the parameters to better match the inputs for the NH3 cubefit call, and use the directory tree assumed there? Then I can easily set up a meta call to run all the non-NH3 fits. In future, this can be revised to use moment maps to find emission to fit.

low-sky commented 7 years ago

Agreed with @rfriesen

I would suggest designing the the code so it can be called with a boolean flag like if gaussFit: right after the ammonia fitting block.

https://github.com/GBTAmmoniaSurvey/GAS/blob/5278f9ff5d539bc512d7ee0631afe913c0616a49/GAS/PropertyMaps.py#L802

jakeown commented 7 years ago

@rfriesen @low-sky Thanks for the feedback. I can certainly make the adjustments you mentioned. I will get to them as soon as I can.

low-sky commented 7 years ago

@jakeown -- Any further changes you want to make here?

jakeown commented 7 years ago

@low-sky -- Yes, I am finally getting back to this. Adjusting the function to move through all the non-NH3 lines in one go, so it can be called after cubefit. I was also using a channel range in the original function to define where to look for the peak, but I will change that to use vmin/vmax as in cubefit.

jakeown commented 7 years ago

@rfriesen @low-sky -- OK, I think the updated gauss_fitter is ready to go. I also added a few lines at the end of cube_fit that will call the Gaussian fitter if a gauss_fit parameter is set to True. I didn't include a mask_function parameter in gauss_fitter like we have in cube_fit, which may be something we need to add later.

I am also using the pprocess module for parallel computations, which means we may need to add that to the dependencies in the README.

low-sky commented 7 years ago

Thanks, @jakeown ! This looks like a great contribution and thanks for factoring in gbtpipe. I was wanting to do that for a while. I read through... but did not TEST the code and it looks fine from my perspective. I would be okay with a merge. @rfriesen ?

rfriesen commented 7 years ago

Hey @jakeown - I'm going through this PR now and am testing it on some of the data - so far, no problems. I'll merge once that's done, hopefully today or tomorrow. Thanks!

rfriesen commented 7 years ago

Ok, @jakeown , looks good except instead of calling gauss_fit.gauss_fitter(region=region, mol=i, vmin=vmin, vmax=vmax, snr_min=snr_min, multicore=multicore, file_extension=file_extension) you should just do gauss_fitter(region=region, mol=i, vmin=vmin, vmax=vmax, snr_min=snr_min, multicore=multicore, file_extension=file_extension) based on your import of gauss_fitter in PropertyMaps.py. I'll merge and then fix.

jakeown commented 7 years ago

@rfriesen Thanks for the catch on the function call and the merge! Hope it is helpful