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 (but properly this time) #42

Closed ehewins closed 1 year ago

ehewins commented 1 year ago

This PR is an alternative approach to fix #40 My original attempt at fixing this issue can be found in PR #41 , but I've opened a new branch and new PR to avoid making a mess of the commit history. The old approach worked, but as @butlerpd pointed out, it didn't go about it in the best way. The new approach found here is based on Paul's suggestions and solves the problem without making the Binning.get_bin_index() method do more than it's supposed to.

Fixes #40

butlerpd commented 1 year ago

I suggest you look at my version with documentation and test. I'm not sure if my version would be easier or not for a new developer 3 years from now to pick up (modulo the documentation some version of which should be included here if this is the version we go with). I'm happy now to approve either way. As I say in the conversation above, unit tests will be important but will take time to get the appropriate data and write so I suggest we DON'T wait for that.

butlerpd commented 1 year ago

I have one other question I completely missed. I do not know phyton or OOP well enough to know the answer. Your code as written changes the value of self.phi_max. Is self.phi_max a purely local variable? or are you making the _Sector class in sasdata change a parameter value that belongs to sasview gui?

If it is local I'm fine. If however we would be changing a WedgeSlicer.py value behind its back so to speak then I think we need to not do that! That would be very bad form I think. In principle it should be ok (2pi differences in angles), but you are changing it here precisely because it does matter in this case so I fear unintended consequences in the future .. particularly by Ellis 2 and beyond 😄. At the least it would probably lead to unnecessary confusion for the next person trying to work in this part of the code base. The fix of course would be trivial -- use the same construct as in version 3 of this branch (passing an expression containing self.phi_max rather than an altered value of self.phi_max. You would also then need to replace self.phi_max with binning.max in the calculation of step (for clarity probably best in that case to also replace self.phi_min with binning.min)

ehewins commented 1 year ago

I can see how all the copping and changing between angular spaces could make code which is already confusing even more confusing. I managed to avoid that confusion first time round by not thinking about it, but ultimately I think the best move would be to cherry-pick your branch's commit into this one, and make a few edits to the comments.

In answer to your other question, as far as I know, this self.phi_min lives entirely within the _Sector class in sasdata. Its initial value is provided by sasview, but sasview doesn't know what we do with it after that.

butlerpd commented 1 year ago

OK - in that case I'm good with it. I would like to see documentation at least in comments before a final merge? Ultimately it would probably be best to move a lot of that into the class doc strings?