search

RascalSoftware / python-RAT

Python interface for RAT
1 stars 5 forks source link

Do we want to split resampleParams into two parameters? #69

Closed alexhroom closed 2 months ago

alexhroom commented 3 months ago

Currently resampleParams is a two item list (default [0.7, 50]), representing a minimum angle (in units 1/pi) and number of points used in resampling. This is very unintuitive and difficult for the user to tell what these parameters represent. If we split them into two parameters, named e.g. resampleMinAngle and resampleNPoints, then we get the following benefits:

Please comment any thoughts, problems, or alternatives?

DrPaulSharp commented 3 months ago

It's worth noting that when resampleParams is used in the matlab (in the routines resampleLayers and resampleLayersReIm) the two elements are immediately split into two separate variables, so the list is only used to package the variables.

alexhroom commented 2 months ago

conclusion from discussion in meeting: yes

alexhroom commented 2 months ago

@arwelHughes there's a discrepancy between MATLAB and Python on how the min angle is validated:

in MATLAB an error is thrown if (val < 0 || val > 1), so values between 0 and 1 (inclusive) are acceptable in Python an error is thrown if not 0 < val < 1 so 0 and 1 are excluded.

I was just wondering which one you thought would be the correct range? do 0 and 1 both make sense as min angles?

arwelHughes commented 2 months ago

Hi Alex, The first is correct, I suspect. But in which function is this? Any checking of this kind would be in ‘adaptive’ itself, which is an unmodified 3rd party function. If it’s an upstream check, I suspect I wouldn’t have put that in (certainly not for Python - the Python is nothing to do with me!!) Cheers, Arwel

From: Alex H. Room @.> Date: Tuesday, 3 September 2024 at 10:36 To: RascalSoftware/python-RAT @.> Cc: Hughes, Arwel (STFC,RAL,ISIS) @.>, Mention @.> Subject: Re: [RascalSoftware/python-RAT] Do we want to split resampleParams into two parameters? (Issue #69)

@arwelHugheshttps://github.com/arwelHughes there's a discrepancy between MATLAB and Python on how the min angle is validated:

in MATLAB an error is thrown if (val < 0 || val > 1), so values between 0 and 1 (inclusive) are acceptable in Python an error is thrown if not 0 < val < 1 so 0 and 1 are excluded.

I was just wondering which one you thought would be the correct range? do 0 and 1 both make sense as min angles?

— Reply to this email directly, view it on GitHubhttps://github.com/RascalSoftware/python-RAT/issues/69#issuecomment-2326058706, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGMNVXXAJPES4SKRRPGV7ATZUV7IFAVCNFSM6AAAAABMYAK2C2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRWGA2TQNZQGY. You are receiving this because you were mentioned.Message ID: @.***>

alexhroom commented 2 months ago

@arwelHughes this is in the function set.resampleParams in the controlsClass, in particular see lines 94-103:

        function set.resampleParams(obj,val)
            if length(val) ~= 2
                throw(exceptions.invalidValue('resampleParams must have length of 2'));
            end

            validateNumber(val, 'resampleParams must be a number array');

            if (val(1) < 0 || val(1) > 1)
                throw(exceptions.invalidValue('resampleParams(0) must be between 0 and 1'));
            end

which it seems Stephen wrote according to git blame. sorry for lack of clarity - was asking more from a "are 0 or 1 sensible values for this parameter" perspective so that I can make it consistent between the two, rather than asking for you to explain something you'd written!

arwelHughes commented 2 months ago

Hi Alex, It is kind of right, but actually thinking about it zero would probably throw an error. Otherwise yes, the Matlab is correct. Cheers, Arwel

From: Alex H. Room @.> Date: Tuesday, 3 September 2024 at 10:58 To: RascalSoftware/python-RAT @.> Cc: Hughes, Arwel (STFC,RAL,ISIS) @.>, Mention @.> Subject: Re: [RascalSoftware/python-RAT] Do we want to split resampleParams into two parameters? (Issue #69)

@arwelHugheshttps://github.com/arwelHughes this is in the function set.resampleParams in the controlsClasshttps://github.com/RascalSoftware/RAT/blob/6fad34d45ce75980547574f679d9f01d5ef6759f/API/controlsClass.m#L94-L103, in particular see lines 94-103:

    function set.resampleParams(obj,val)

        if length(val) ~= 2

            throw(exceptions.invalidValue('resampleParams must have length of 2'));

        end

        validateNumber(val, 'resampleParams must be a number array');

        if (val(1) < 0 || val(1) > 1)

            throw(exceptions.invalidValue('resampleParams(0) must be between 0 and 1'));

        end

which it seems Stephen wrote according to git blame. sorry for lack of clarity - was asking more from a "are 0 or 1 sensible values for this parameter" perspective so that I can make it consistent between the two, rather than asking for you to explain something you'd written!

— Reply to this email directly, view it on GitHubhttps://github.com/RascalSoftware/python-RAT/issues/69#issuecomment-2326105051, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGMNVXV5FSS3EAIKBQQUHZDZUWB4DAVCNFSM6AAAAABMYAK2C2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRWGEYDKMBVGE. You are receiving this because you were mentioned.Message ID: @.***>

DrPaulSharp commented 2 months ago

I'm more than happy to change the python to match the matlab, but I think it might be worth running the examples with 0 and 1 for minAngle and seeing what happens, and in particular if there is an error. The half open interval 0< minAngle <=1 may be the way to go here.

alexhroom commented 2 months ago

decided on 0 < minAngle <= 1 via looking at the algorithm: the algorithm resamples if the (smaller) angle between the line segments $(x_{n-1}, x_n)$ and $(xn, x{n+1})$ is strictly less than minAngle, which means 0 is not a sensible value (it just won't do any resampling at all). on the other hand if minAngle is 1, it means it will resample all points.