Zabamund / wellpathpy

Well deviation import
GNU Lesser General Public License v3.0
78 stars 28 forks source link

Azimuth Must Be Strictly Less Than 360 #34

Closed dmbaker closed 3 years ago

dmbaker commented 3 years ago

Current:

if not ((0 <= azi) & (azi <= 360)).all():
        raise ValueError('all azi values must be in range 0-360')

Should be:

if not ((0 <= azi) & (azi < 360)).all():
        raise ValueError('all azi values must be in range GTE 0 and LT 360')
Zabamund commented 3 years ago

Commit 022eb07 agrees with this issue and applies a fix as suggested.

jonnymaserati commented 3 years ago

Alternatively, you could use the modulo operator and avoid the risk of raising an error completely:

azi %= 360
jokva commented 3 years ago

That's semantically quite different - one (the error) signals that the input is probably bad, while the other silently accepts it.

jonnymaserati commented 3 years ago

It's not wrong, it's just not good practice. Could give a warning rather than breaking since it's a shame to waste some good compute?

jokva commented 3 years ago

Personally I think the right choice is to fail, and require data to be sanitized. It's not too bad for callers to do a % 360 either, and that way pipelines will fail rather than (somewhat quietly) warn should there be a problem.

Good practice is failing.

jonnymaserati commented 3 years ago

Agree that users should sanitize their own input data.

On 5 Aug 2021, at 17:53, Jørgen Kvalsvik @.***> wrote:

 Personally I think the right choice is to fail, and require data to be sanitized. It's not too bad for callers to do a % 360 either, and that way pipelines will fail rather than (somewhat quietly) warn should there be a problem.

Good practice is failing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.