Throw appropriate error for pad_sequence input of length > maxlen #186
Conversation
|
Thanks sean-gauss, (For record-keeping purposes) This PR fixes #183 |
There was a problem hiding this comment.
@sean-gauss Please, do let me know if you need any help with this?
src/sentiment.jl
Outdated
| @@ -8,6 +8,11 @@ function pad_sequences(l, maxlen=500) | |||
| push!(res, ele) | |||
| end | |||
| return res | |||
| else | |||
| throw(1) | |||
| print("String exceeds maximum length") | |||
There was a problem hiding this comment.
I am afraid that this will throw something like this - ERROR: 1 and will never execute the print statement even if it executes the else-block.
Try looking for docs (online or offline) for error and throw. You can easily display error message using those APIs.
There was a problem hiding this comment.
@Ayushk4 I have made the changes desired and the function seems to be working fine as when I call it by modifying the maxlen variable it shows the desired Output.
Also I was wondering if XLNet is implemented for Julia as I think it will be a reasonably good addition
There was a problem hiding this comment.
Yes, XLNet will be a good addition. You can check out Flux.jl and the transformers are in transformers.jl.
|
@Ayushk4 Hey, I was interested in working on this issue, can I work on completing this PR? |
| @@ -8,7 +8,9 @@ function pad_sequences(l, maxlen=500) | |||
| push!(res, ele) | |||
| end | |||
| return res | |||
| end | |||
| else | |||
| length(1) > maxlen || throw("String length exceeds maximum length") | |||
There was a problem hiding this comment.
length(1) => length(l)?
Also, why are we checking this condition again? length(l) > maxlen
There was a problem hiding this comment.
Should the checking condition be &&? even in that case?
| end | ||
| else | ||
| length(1) > maxlen || throw("String length exceeds maximum length") | ||
| end |
There was a problem hiding this comment.
| end | |
| end |
Indentation
|
@sean-gauss, it's great that you are putting in your time and effort into fixing this bug. But, I am afraid your code seems to have some unnecesarry condition checking. Read here - https://docs.julialang.org/en/v1/manual/control-flow/#Short-Circuit-Evaluation-1 The following code should work (do test it locally, once as well) While you are at it, it will be great if you can also add a test for the same. Feel free to post your doubts here regarding the PR. |
|
Bump @sean-gauss . |
|
Yes @Ayushk4 |
Displays an error message in case length of string is greater than the defined maximum length.