search

BUNPC / Homer3

MATLAB application for fNIRS data processing and visualization
92 stars 54 forks source link

Input argument validation #104

Closed Horschig closed 9 months ago

Horschig commented 2 years ago

Dear Homer3 team,

in collaboration with Vijay from the Mathworks team, we have worked on some Homer3 improvements. This is the first pull request which we are making. It is featuring the new input validation feature of Matlab (https://www.mathworks.com/help/matlab/matlab_prog/function-argument-validation-1.html).

Note that the syntax is not supported by older Matlab versions. Vijay would like to chime here, so I will notify him.

sstucker commented 2 years ago

Thank you for this excellent contribution.

I have the following comments on the code:

It would be better to refactor the isExpectedClass function and any other repeated code into a separate function, perhaps in Utils.

This would require that the new release target 2021b... This should be okay? @jayd1860

vijayiyer05 commented 2 years ago

@sstucker Glad function argument validation is appearing a useful direction to make Homer code more readable and robust.

Regarding minimum version, did you identify a particular aspect that needed R2021b? Function argument validation was first introduced in R2017a. The most recent validation functions appear to have been added in R2020b.

sstucker commented 2 years ago

@vijayiyer05 Ah, okay. I am only concerned with the dependence of this code. Which is > 2017a?

vijayiyer05 commented 2 years ago

@sstucker That makes sense. Can't say for sure since I haven't analyzed to see if there's a dependency on a newer validation function. My suggestion would be to try the Code Compatibility Report feature/function to help make the determination.

Horschig commented 2 years ago

Thank you for this excellent contribution.

I have the following comments on the code:

It would be better to refactor the isExpectedClass function and any other repeated code into a separate function, perhaps in Utils.

This would require that the new release target 2021b... This should be okay? @jayd1860

That is an excellent suggestion. However, you guys have to decide whether you want this in at all, because the syntax breaks backwards compatibility:

image

If you are planning to accept this PR, then we are happy to refactor. Otherwise, it'd be a waste of time obviously ;)

edit: the attached screenshot is from Matlab 2019b and shows that it refuses to work with a function in that style.

Horschig commented 2 years ago

@vijayiyer05 Ah, okay. I am only concerned with the dependence of this code. Which is > 2017a?

I think it is 2021b:

image

vijayiyer05 commented 2 years ago

@Horschig: this version number at top simply indicates which version of the documentation this is. By default, one looks at the latest.

To see when a feature was introduced, one can look at a specific function doc page and scroll to the bottom to see what version introduced it.

Above I noted that validation functions were introduced in R2017a.

Looking a bit closer, this was initially for class properties. It seems the arguments block for functions was introduced in R2019b.

So 2019b should be the min version dependency to incorporate this. There were some new validators introduced in R2020b, but those are for convenience and not an essential dependency.

Note that I generally recommend keeping a "future" branch that includes a mix of things that are improvements for the developers and users respectively. When you have something that's enough of an enticement for a new user to upgrade Homer, I think that's the best time to ask them to update their foundations as well.

sstucker commented 2 years ago

I strongly think we should shelve any changes that require an update beyond 2017b for now. Anything else would be great to get merged.

dboas commented 10 months ago

@Horschig I could go ahead and approve this. It looks okay to me. Can you just double check it and confirm you want me to proceed with this. We are getting back on top of things here.

Horschig commented 9 months ago

Thanks for getting back to this. I think it is up to you. This change breaks backwards compatibility with older Matlab versions, from what I see Matlab 2019b or newer will be required. I cannot judge whether you want that, as it might result in numerous support questions like "why does Homer gives an error". On the other hand, it would simplify the code, make it more robus and make maintenance somewhat easier. Your call, I won't be mad either way ;)

dboas commented 9 months ago

We are fine not supporting before 2019b Thx

From: Sreekanth kura @.> Date: Wednesday, January 17, 2024 at 2:14 PM To: BUNPC/Homer3 @.> Cc: Boas, David @.>, Comment @.> Subject: Re: [BUNPC/Homer3] Input argument validation (PR #104)

Merged #104https://github.com/BUNPC/Homer3/pull/104 into development.

— Reply to this email directly, view it on GitHubhttps://github.com/BUNPC/Homer3/pull/104#event-11515431352, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHFCP5H5TQSOA3UP2T7B5K3YPAPIPAVCNFSM5MJYDOWKU5DIOJSWCZC7NNSXTWQAEJEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW4OZRGE2TCNJUGMYTGNJS. You are receiving this because you commented.Message ID: @.***>