pypeit / PypeIt-development-suite

8 stars 11 forks source link

NIRES wavecalib is slow #54

Closed feigewang closed 6 years ago

feigewang commented 6 years ago

The wavecalib seems quite slow for NIRES. It works ok for first two orders (i.e. Y/J bands), but getting slower in H and almost failed in K band (last order). I upload a notebook under development suite dev_nries_run branch named as NIRES_WaveCalib.ipynb. @rcooke-ast Please check it out. Thanks!

rcooke-ast commented 6 years ago

Hi @feigewang - how slow is quite slow? Note that there is a significant overhead running notebooks, so it's much faster if you run it in a terminal. Also, it wasn't clear from your message - did any solutions fail? I would say 3/4 solutions is pretty good! :-D

jhennawi commented 6 years ago

Hi @rcooke-ast,

The older versions of arclines were already nailing these wavelength solutions. The new version gets it but K-band takes like 2 hours. I think this is not a notebook issue, while I agree that benchmarking should not be done in notebooks.

Thanks for looking into this.

Joe

feigewang commented 6 years ago

Hi @rcooke-ast , Thanks for looking into this. The wavecalib gives you pretty good solutions (i.e. better rms than old PYPIT version). But, as Joe said, it took 10-20ish minutes in H and couple hours (maybe even longer) to get the solution in K. It is not a notebook issue, as I have been running it in terminal. The notebook was just made for help looking into this.

Thanks, Feige

rcooke-ast commented 6 years ago

Hi @feigewang - I've addressed this (I hope!) in the PR available for review in the main repo. Try it out, and let me know if this improves the speed a little. You may need to play around with some parameters in the autoid file. I haven't done an exhaustive test, but I would start with the idthresh fraction if the speed increase is still not sufficient.

feigewang commented 6 years ago

Hi @rcooke-ast , Thanks so much. I checked it out. The speed does much faster than previous version. But the solution was far off for NIRES order 1 due to the peak detection problem. @jhennawi made some modification on finding peaks and can improve this.

Cheers, Feige