Major bug fixes in pydlutils/bspline.py

[jhennawi]

In the process of porting xidl spectroscopic data reduction codes into python, I discovered and fixed many issues/bugs in the pydl bspline port. I've also added a new method to the bspline class workit which ports the ~idlutils/pro/bspline/bspline_workit.pro function in idlutils. This was needed to do run a piece of code Scott buries wrote (bspline_extract and/or bspline_longslit) which are used in other data reduction pipelines.

We would like to make pydl a dependency of the PYPIT data reduction framework (https://github.com/PYPIT/PYPIT), which will result in more users checking and fixing the codes.

[weaverba137]

Thank you for submitting this PR, but it looks like existing unit tests have been broken. Once those are fixed, I'll take a look. Also, please document all of the specific bugs you found, because, as far as I was aware, the bspline code was working fine for the purpose it was written, which was assembling SDSS spectroscopic templates. The file docs/pydl/changes.rst would be the place for this.

[jhennawi]

Hi Ben,

I'm happy to fix the unit tests. These mostly result from the fact that I slightly changed the arguments to iterfit to be able to pass a distinct set of kwargs to djs_reject, and to distinguish those from the list of kwargs passed to the bspline upon instantiation in order to determine the breakpoints.

Your bspline implementation port had major bugs in it which I've fixed. Detailing all those fixes in changes.rst would be a bit tedious but I can do it if you really think it is necessary. You can easily see them by doing a code diff. You may not have noticed all of these as many of them are in the x2 polynomial fit implementation, and my guess is you rarely used or tested that.

That said your iterfit does not produce correct results anywhere because it never iterates. On line 614 of bspline.py you have:

while (error != 0 or qdone == -1) and iiter <= maxiter

Later on the code calls djs_reject and qdone is set to a boolean. In IDL -1 is False. In python it is not. The upshot is that the code will never perform more than one iteration because even if djs_reject returns False, that is never == -1.

I can see how you would miss this, often one bspline without iteration or rejection is enough, but your code is not working in the same way as the IDL version. The rejection is broken, and so it does not produce the right results.

Please let me know how to proceed with this PR.

Thanks, Joe

[weaverba137]

Actually, that description would be perfect for the change log, you could even condense it a little bit, and just link back to this PR. But please fix the unit tests. I'm just following the same standards that the Astropy project does, since PyDL is an Astropy-affiliated package.

Is it also possible for you to confirm that the revised bspline code gives the same results (up to some reasonable level of precision) as the IDL code? That is another important standard for PyDL, that it give the same numbers. This should be included in the unit tests.

Finally, you are certainly welcome to include PyDL as a dependency for PYPIT. PyDL itself would not bring any dependencies that you don't already have. And it looks like PYPIT is already Python 3 compatible, so that's good. Just like Astropy itself, I'll drop support for Python 2 in the relatively near future.

[jhennawi]

Dear @weaverba137,

We discussed how best to integrate pydl into the PypeIt (https://github.com/pypeit/PypeIt ) project with my collaborators. The were however loathe to add pydl as a dependency. The path we have adopted is to have a pydl module in PypeIt. This is now where the bug fixes to pydl live for select routines. The bspline implementation had tons of bugs in it, which have now all been fixed in pydl.

I'm happy to discuss the best path forward with you, and would personally prefer if we could make pydl a dependency of PypeIt. This would however require that you accept all of our fixes, and also give one us write acess to the pydl repository. Happy to discuss further.

We would also probably need your help to pull of this merge. Note however, that a bunch of other idlutils and idlspec2d codes have been ported by us, so there is a lot to gain here.

Best Wishes, Joe Hennawi

[weaverba137]

I've already mentioned some of this, but let me reiterate:

In order to accept the fixes on this existing PR:

1. All tests need to pass, and the test coverage should increase or at least remain the same.
2. I need to see test code that demonstrates that the changes to bspline still work with iterations turned off, and give the same numeric results as the IDL code in all cases.
3. There needs to be documentation of the change in the change log file. You can scrape the text off of this PR discussion, but the documentation needs to be there.

I don't understand why you need write access to the repository. It is a perfectly normal workflow to fork the repository and submit pull requests upstream. I am an official developer on Astropy, and I never make changes to the primary repository. I submit changes through my personal fork.

[weaverba137]

You might want to take a look at the Astropy Developer Documentation. I try to follow those standards as closely as possible. I particularly keep the documentation and testing standards open whenever I'm working on PyDL code.

[weaverba137]

@jhennawi, I get that you're busy and not interested in trying to merge any changes, but could you please at least give me some test data that demonstrates the problem and its resolution?

[jhennawi]

Hi @weaverba137 the number of bugs was really significant and I did not take detailed notes. I think the best way to find them would be to diff the pypeit.core.pydl module with your codes. I also added some functionality that I needed to the bspline class so you may not agree with all of the changes.

Joe

[weaverba137]

@jhennawi, please understand. I'm not asking for code. I can do a diff. I need DATA to demonstrate the bugs and their resolution. Please give me the DATA you used to find these bugs. Is that clear?