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

Throw appropriate error for pad_sequence input of length > maxlen #186

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sean-gauss
Copy link

Displays an error message in case length of string is greater than the defined maximum length.

src/sentiment.jl Outdated Show resolved Hide resolved
src/sentiment.jl Outdated Show resolved Hide resolved
@Ayushk4
Copy link
Member

Ayushk4 commented Jan 16, 2020

Thanks sean-gauss,
A few changes and this should be ready to go.

(For record-keeping purposes) This PR fixes #183

Copy link
Member

@Ayushk4 Ayushk4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sean-gauss Please, do let me know if you need any help with this?

src/sentiment.jl Show resolved Hide resolved
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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are examples that might help you - [1], [2]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, XLNet will be a good addition. You can check out Flux.jl and the transformers are in transformers.jl.

@amsuhane
Copy link

@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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

length(1) => length(l)?

Also, why are we checking this condition again? length(l) > maxlen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the checking condition be &&? even in that case?

end
else
length(1) > maxlen || throw("String length exceeds maximum length")
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
end
end

Indentation

@Ayushk4
Copy link
Member

Ayushk4 commented Feb 26, 2020

@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)

    else
        throw("String length exceeds maximum length")
    end

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.

@Ayushk4
Copy link
Member

Ayushk4 commented Jul 24, 2020

Bump @sean-gauss .

@sean-gauss
Copy link
Author

Yes @Ayushk4

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

Successfully merging this pull request may close these issues.

3 participants