Do we want to split resampleParams into two parameters?

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:

easier to document and understand what these parameters are from a user point-of-view
easier to input in both GUI (min angle can be a float spinbox and npoints an int) and in scripts
easier to validate (in Python, can let Pydantic handle it entirely and also add handling to ensure n-points is an int)
do not have to write specialised GUI code just to handle a 2-item list once in the Controls tab

Please comment any thoughts, problems, or alternatives?

conclusion from discussion in meeting: yes

@arwelHughes

@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?

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) @arwelHughes<https://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 GitHub<#69 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGMNVXXAJPES4SKRRPGV7ATZUV7IFAVCNFSM6AAAAABMYAK2C2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRWGA2TQNZQGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

@arwelHughes

@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!

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) @arwelHughes<https://github.com/arwelHughes> this is in the function set.resampleParams in the controlsClass<https://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 GitHub<#69 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGMNVXV5FSS3EAIKBQQUHZDZUWB4DAVCNFSM6AAAAABMYAK2C2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRWGEYDKMBVGE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

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 $(x_n, 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.

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

alexhroom commented on Sep 2, 2024

alexhroom commented on Sep 3, 2024

arwelHughes commented on Sep 3, 2024

alexhroom commented on Sep 3, 2024

arwelHughes commented on Sep 3, 2024

alexhroom commented on Sep 6, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions