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

Input warning not helpful and problem with numpy data types #217

Open
tbalke opened this issue Nov 13, 2021 · 4 comments
Open

Input warning not helpful and problem with numpy data types #217

tbalke opened this issue Nov 13, 2021 · 4 comments
Assignees

Comments

@tbalke
Copy link
Collaborator

tbalke commented Nov 13, 2021

It seems very reasonable to iterate over parameter arrays such as

num_threads = np.array([2, 4]) # num_threads[n] is of type numpy.int64

for n in range(2):
   result[n] = svmbir.recon( ..., num_threads=num_threads[n])

However this results in the somewhat useless warning:
UserWarning: Parameter num_threads is not a valid int. Setting to default.

The warning comes from _utils.py:

    if not (isinstance(num_threads, int) and (num_threads > 0)):
        warnings.warn(f"Parameter num_threads is not a valid int. Setting to default.")
        num_threads = None

Problems with this:

  1. First I thought that
    UserWarning: Parameter num_threads is not a valid int. Setting to default.
    will take my 2 (numpy.int64) and convert it to 2 (int). But instead it will completely disregard the 2 and set it to the number of cores!!!

In my opinion the hard setting to the defaults whenever an input is not precisely what is expected seems very strict and can lead to unexpected behavior.

  1. Whenever a variable get checked it is advised to display the variable and type. Something like
    warnings.warn(f"Parameter num_threads ({num_threads}, {type(num_threads)}) is not a valid int. Setting to default.")
    would give UserWarning: Parameter num_threads (2, <class 'numpy.int64'>) is not a valid int. Setting to default. Now it is so much easier to understand what is wrong.

  2. when num_threads=None this warning is still displayed and then num_threads is again set to None.

  3. In my opinion numpy.int64 should be a perfectly reasonable input. Why not do
    isinstance(num_threads, (int, np.int64, np.int32))? There may also be more elegant ways of doing this.

  4. The logic in the if clause is not obvious. Can there be more parentheses?

  5. In _utils.py there are more instances like this for other parameters that should be improved.

@sjkisner
Copy link
Collaborator

I already fixed this in the open pull request.

@sjkisner
Copy link
Collaborator

Wait sorry. I didn't fix this..
What I did fix just now was the same warning about num_threads but it was thrown for a different reason.

@tbalke
Copy link
Collaborator Author

tbalke commented Nov 15, 2021

In general, it often happens that None is passed as an argument as in

kwargs = {'positivity': True}
...
recon( ...
    positivity=kwargs.get('positivity'), # returns True
    max_iterations=kwargs.get('max_iterations'), # returns `None` and causes a warning
)
...

@bwohlberg Can you comment?

@bwohlberg
Copy link
Collaborator

Instead of issuing warnings when parameters such as stop_threshold are passed a None value, I would suggest just silently setting those parameters to their default values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants