Closed arunkannawadi closed 4 months ago
arunkannawadi
commented
4 months ago It looks like @esheldon (re-)recognized the point I was trying to make here himself in #218 (comment). In either case, all of the changes proposed here are subsumed in there, so closing this PR without merging.
esheldon
commented
4 months ago Yes, sorry about that. I should have remembered this PR
arunkannawadi
commented
4 months ago I don't mind this PR not going in, but now that you got the point of the PR, I want to use this opportunity to know how I could have conveyed that point across better. The proposed change wasn't really a change at all, but restoring the behavior of the code as it was before something changed in the dependency (LSST DM code in this case). I did not expect it to take 10+ messages in the conversation, and still not able to convince you. What should I have done better?
beckermr
commented
4 months ago That's a good question. From my point of view, it is my own lack of understanding of how the dm changes came about and if we were papering over some bigger issue.
I tend to be super conservative on changes like this since I've been burned more than once in terms of breaking shear inference.
My apologies for any trouble I have caused here and I appreciate the direct communication!
esheldon
commented
4 months ago I think I had mentally "handed this off" to Matt to finish the debate, and then forgot about it
arunkannawadi
commented
4 months ago OK, thanks for the explanation. I want to assure you that I don't propose changes to the default branch lightly, especially when it involves shear.
A few months ago, some of the default values for the detection config were changed in the LSST DM codebase. In particular, this commit broke one of the tests. The change in the PR is the minimal change required for the tests to pass again.