- 
                Notifications
    
You must be signed in to change notification settings  - Fork 577
 
Enable specifying reference direction for azimuthal angle in PolarAzimuthal distribution #3582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 
           I noticed the point detector work would benefit from this PR and wondered if it is worth putting in a couple of tests. I can PR to your branch if that helps def test_reference_vwu_projection():
    """When a non-orthogonal vector is provided, the setter should project out
    any component along reference_uvw so the stored vector is orthogonal.
    """
    pa = stats.PolarAzimuthal()  # default reference_uvw == (0, 0, 1)
    # Provide a vector that is not orthogonal to (0,0,1)
    pa.reference_vwu = (2.0, 0.5, 0.3)
    reference_v = np.asarray(pa.reference_vwu)
    reference_u = np.asarray(pa.reference_uvw)
    # reference_v should be orthogonal to reference_u
    assert abs(np.dot(reference_v, reference_u)) < 1e-6
def test_reference_vwu_normalization():
    """When a non-normalized vector is provided, the setter should normalize
    the projected vector to unit length.
    """
    pa = stats.PolarAzimuthal()  # default reference_uvw == (0, 0, 1)
    # Provide a vector that is neither orthogonal to (0,0,1) nor unit-length
    pa.reference_vwu = (2.0, 0.5, 0.3)
    reference_v = np.asarray(pa.reference_vwu)
    # reference_v should be unit length
    assert np.isclose(np.linalg.norm(reference_v), 1.0, atol=1e-12) | 
    
| 
           Thanks @shimwell. I invite you to push the tests to my PR.  | 
    
          
 PR made GuySten#33  | 
    
added ref vwu tests
| 
           @shimwell, are you interested in reviewing this PR as well?  | 
    
| 
           Yes sorry I did take a look at everything and those tests (thanks for fixing) were the only thing I thought it was missing. So this looks good to me. LGTM  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to chime in too late here -- the name reference_vwu doesn't really make sense to me. I'll submit a follow-on PR to suggest a new name.
Description
Currently the reference direction for the azimutal angle in PolarAzimuthal distribution is not clear.
This PR enable specifying the direction in the python API so it is clear what is going on and what is the direction that corresponds to a specific phi
Checklist