Skip to content
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

Add parameter sanity checking to all functions/modules #108

Open
adrianVmariano opened this issue Sep 18, 2019 · 7 comments
Open

Add parameter sanity checking to all functions/modules #108

adrianVmariano opened this issue Sep 18, 2019 · 7 comments
Labels
Enhancement New feature or request
Milestone

Comments

@adrianVmariano
Copy link
Collaborator

Someone mentioned this issue on the forum, and it's a real issue. When you pass the wrong type of parameter to a function/module you often get mysterious errors that are hard to understand.

I think that in any case where it doesn't cripple performance we should add full parameter checking. I was thinking we could have functions like

module assert_list(name,parm) { 
    assert(is_list(parm),
               str("Parameter ",name," must be a list in ",parent_module(1)))
}

Or maybe we make a master type checker function that takes a list of types like ["number","vector"] and allows any one of those types.

Also when parameters are redundant, that should be an error. I've implemented cases like this, but I don't think it's systematic. In other words, if you specify r and d you don't pick one...you give an error.

@revarbat
Copy link
Collaborator

Currently, in cyl(), if you specify r and one of r1 or r2, then the value of r will be used for the more specific r1/r2 that wasn't specified. ie: cyl(l=10,r=10,r2=5); is the same as cyl(l=10,r1=10,r2=5);, which is the same as cyl(l=10,r1=10,r=5);

Do we want to forbid this usage and force r1 and r2 to be used together, and assert that r is not used with r1 or r2?

@adrianVmariano
Copy link
Collaborator Author

I think the point is we want to catch user input errors and flag them. I don't want to spend an hour trying to figure out why

cyl(r=1, h=10, d=9);

is producing a 2 unit diameter result no matter how I set d because I just didn't notice that I also passed in r. We also don't want functions/modules to bomb with cryptic backtraces because the code required a list and someone past a scalar.

In the case of r1 and r2 for cyl() I think that d or r (but not both) should be ok if only one of r1 and r2 is defined, but if both r1 and r2 are defined and either of d or r is defined it should be an error. Basically if there is more than one potential interpretation of the input, it should be an error.

@revarbat
Copy link
Collaborator

I do think we want to allow mixing r1/d2 or d1/r2

@adrianVmariano
Copy link
Collaborator Author

That's fine. I think that as long as the input is unambiguous, it's fine. But if the input is incomplete or ambiguous then there should be an error.

@revarbat
Copy link
Collaborator

I've committed a tweak to get_radius() to make above said checks.

@adrianVmariano
Copy link
Collaborator Author

I noticed that get_radius() is used by a bunch of functions that look like they take different args. Does your tweak work universally?

@revarbat
Copy link
Collaborator

I went through all the calls to it, and I'm pretty certain it'll work as expected.

@revarbat revarbat added the Enhancement New feature or request label Apr 10, 2020
@revarbat revarbat added this to the v2.0.0 beta milestone May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants