-
Notifications
You must be signed in to change notification settings - Fork 45
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
IEC TS 62600 updates #382
IEC TS 62600 updates #382
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akeeste thanks for updating the functions to match the latest TS.
The changes are consistently applied and the only question I had was around if in the future we would want to deprecate capture length. You captured this option in your PR summary. Interested in capturing your thoughts here before merging this PR.
@ssolson I'm on the fence. It's not a lot of effort to maintain these two so I'm in no rush to deprecate them, but I know this stuff adds up over time. Perhaps we should add a |
I think we should aim to support only the terminology defined in the standard. I agree that we should add a future deprecation warning. |
Sounds good. I'll add two |
@ssolson is there a reason we ignore all FutureWarnings in |
@ssolson I added FutureWarnings. Per https://docs.python.org/3/library/warnings.html, this seems like the most accurate category to use. However we ignore all of these warning types in mhkit/init.py which prevents users from seeing it |
@akeeste apologies I missed the notification on this and I should have checked in sooner. I believe ignoring the future warning was added to address mhkit dependencies warnings which could not be updated at one time (e.g. the update needed to remove the future warning was not available). Seeing the warning added confusion to the user's mhkit environment setup we did not want them to worry about. Now that the future warning exists in this module I suggest we merge this PR and in a separate issue/ PR we handle cleaning up this logic to address only specific future warnings that we specifically want to ignore (replacing the blunt ignore all logic currently implemented). |
@ssolson that sounds good. This is ready to merge from my end. The FutureWarnings currently added will work once we address ignoring them over all |
Resolves #381
TODO:
L
-->CW
,LM
-->CWM
)capture_length
andcapture_length_matrix
that point tocapture_width
andcapture_width_matrix
respectively.