3
\$\begingroup\$

I am not a developer, but I needed to write a script.

So I wrote it. Since I can't find an accessible tool that fulfills the same purpose, which I think may be helpful to others, I decided to publish it: https://github.com/gabriel-de-luca/simil/blob/master/simil.py

I am open to review it completely, but I would particularly like to consult on the way I am checking the validity of the data.

The main function is called process and has two positional parameters: source_points and target points, of array_like (numpy) type.

The first thing I do is convert the inputs into numpy arrays and transpose them, because all the code requires one row per coordinate (X, Y and Z) and one column per point.

In addition to transposing it, I verify that it has three coordinates, and if it has just two coordinates I allow myself to fill the Z with zeroes.

import numpy as np

def _get_coords(points):
    coords = np.array(points, dtype=float, ndmin=2).T
    if coords.shape[0] != 3:
        if coords.shape[0] == 2:
            coords = np.concatenate((coords, np.zeros((1, coords.shape[1]))))
        else:
            raise
    return coords


def process(source_points, target_points, alpha_0=None, scale=True, lambda_0=1):
    """
    Find similarity transformation parameters given a set of control points
    """

    print('Processing...')

    # false outputs to return if an exception is raised

    false_lambda_i = 0
    false_r_matrix = np.array([[0, 0, 0],
                               [0, 0, 0],
                               [0, 0, 0]])
    false_t_vector = np.array([[0], [0], [0]])

    # checks

    try:
        source_coords = _get_coords(source_points)
    except:
        print('ERROR:\n' + 
              f'source_points = {source_points}' +
              '\n could not be broadcasted as a numerical array' +
              '\n with shape = (n, 3).')
        return false_lambda_i, false_r_matrix, false_t_vector

Then, the code continues to do other checkups, and if everything goes well, it returns valid values in the output variables: 

    print('Done.')

    return lambda_i, r_matrix, t_vector

If I did not do this check, many errors of different types could occur in other private functions when the data is processed.

I run this script from other scripts, and this is what happens if I send invalid data: 

import simil

source_points = [[0]]

target_points = [[1]]

m, r, t = simil.process(source_points, target_points)

print('\nm value returned = ' + str(m))

Returns: 

Processing...
ERROR:
source_points = [[0]]
 could not be broadcasted as a numerical array
 with shape = (n, 3).

m value returned = 0

It works fine to me, because I can check if m == 0 in the other script to stop it, but I am publishing my script and I don't know if this is the right way to handle the exception, or how to improve it.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Code Review! Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ Commented Mar 6, 2020 at 15:07
  • \$\begingroup\$ @TobySpeight, The purpose of the code would be the evaluation of the input values? A thousand apologies but I don't understand what you mean. Feel free to edit the title of my question or please try to be more specific. \$\endgroup\$ Commented Mar 6, 2020 at 19:57

1 Answer 1

Reset to default
5
\$\begingroup\$

To me you're kind of handling the exception in two ways:

  • you are acting like a library in the sense that you talk about an exception to be raised;
  • you are acting like an application in the sense that you print out error messages and then continue running.

I think you are mainly having the script / function be used as a library, as you return the matrix rather than printing it out. If you talking about handling the exception for a library function, which seems most reasonable, then you should raise an exception rather than returning a magic value (m = 0) that you can check. Make sure that the exception has a clear message that may be put into a stacktrace in case it isn't handled. So you catch and re-raise.

In your case it is probably best to throw a more specific exception in _get_coords though, e.g.:

raise ValueError(f'source_points = {source_points}' +
                 '\n could not be broadcasted as a numerical array' +
                 '\n with shape = (n, 3).')

It works fine to me, because I can check if m == 0 in the other script to stop it

That's exactly what exceptions are for. Just let the other script catch it and stop. The situation could be different if you would want to keep on running with a different matrix.

Magic values are often unclear to the reader, and may be skipped on accident. Try to avoid them as much as possible. You don't want to run on with the magic value as invalid state, so raise and make sure you don't.

The same thing happens when you print Processing... to the output stream. Generally library functions don't print to the output stream. If you want to have a progress indication, then allow some kind of progress reporting function to be present as parameter or class field, and report progress that way. If you just want to indicate that it started progressing then you should however just let the user of the library report that a function is called.

Using a logger rather than output stream would be highly advisable.

You can find more information about listeners here. Logging is also mentioned over there, which is likely not coincidental.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for your answer. It would be very helpful for me if you could also exemplify, from: "then you should raise an exception ..." to "... that the exception has a clear message" , about how to impement it in the code. \$\endgroup\$ Commented Mar 6, 2020 at 10:09
  • \$\begingroup\$ No need to be in my code, just an example on a function about how to do it. \$\endgroup\$ Commented Mar 6, 2020 at 10:12
  • \$\begingroup\$ @MaartenBodewes The easiest way to get access to numpy and lots of other useful scientific Python packages is to use something like Anaconda Python, in case you haven't heard of it. The process is relatively painless and fast ;-) \$\endgroup\$ Commented Mar 6, 2020 at 12:00

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.