search

SasView / sasdata

Package for loading and handling SAS data
BSD 3-Clause "New" or "Revised" License
1 stars 2 forks source link

Fixing Issue #40 - SectorPhi class throws errors (bin index out of range) #41

Closed ehewins closed 1 year ago

ehewins commented 1 year ago

This commit makes the necessary changes to fix the issues with "Wedge Averaging in Phi" using the Wedge Slicer, where certain phi values would cause a 'bin index out of range' error. The method Binning.get_bin_index() from manipulations.py now checks if the index it's about to return is bad, and corrects it if so (using an improved version of the flip_phi function).

Fixes #40

The steps needed to produce the original error have been followed, and the error is now fixed. "Wedge Averaging in Q" was also tested to see if these changes introduce errors in other slicers using the new get_bin_index() method, but no problems were found.

ehewins commented 1 year ago

Hmm. That's quite a lot of failed checks. Time to do some digging.

ehewins commented 1 year ago

Right, it looks like the tests failed because of my re-implementation of the flip_phi() method. Returning the method to its previous state allows the unit test to succeed, but I didn't like the fact it was using an open interval. If I make it a half-open interval by changing one of the <s or >s to a <= or a >= however, the unit test starts to fail again. It seems that with such a setup, the function thinks you've got an angular range of 0° when you're actually trying to use 360°.

ehewins commented 1 year ago

My last commit fixes the broken unit test, but I've discovered another bug which needs to be fixed. I think said bug has always been present, but was hidden by the one this PR set out to fix. The problem is this: you can't set delta_phi [deg] to 180 degrees (in the 'Edit Slicer Parameters' menu for Wedge Averaging in Phi), for the same reason I gave at the end of my previous comment. I have a feeling the solution will be in the code for the Ring class, since it's effectively doing the same job when the angles are set like this.

ehewins commented 1 year ago

My last commit fixes the broken unit test, but I've discovered another bug which needs to be fixed. I think said bug has always been present, but was hidden by the one this PR set out to fix. The problem is this: you can't set delta_phi [deg] to 180 degrees (in the 'Edit Slicer Parameters' menu for Wedge Averaging in Phi), for the same reason I gave at the end of my previous comment. I have a feeling the solution will be in the code for the Ring class, since it's effectively doing the same job when the angles are set like this.

I've noticed that this bug also applies to Wedge Averaging in Q. It seems you can't use the wedge slicer with a full circle ROI under any circumstances. Considering the problem mentioned in Issue #40 has been fixed here, I really ought to open a new issue and continue with the wedge slicer full-circle bug there.