-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add natural language input #25
base: main
Are you sure you want to change the base?
Conversation
mkg33
commented
Jun 17, 2022
•
edited
Loading
edited
- Added a function that converts ACE input to a TPTP formula using the Attempto webservice.
- Added a function that converts a TPTP formula to natural language.
- Added an example that illustrates the usage.
- Also corrected a few minor typos.
(ignore the failing CI for now; as long as it compiles it is OK) |
Great, that's good to know! |
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.
Just a few very minor remarks and suggestions. This is some very good Scala code already!
var translation = "" // translation result | ||
var errorCode = false | ||
|
||
breakable { |
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.
Interesting, I've never seen nor used any of scala.util.control
; thanks for the discovery 🙂
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.
I was actually surprised that break
can't be used without the import but then read somewhere that the philosophy behind it is that Scala doesn't want to include 'too much'. At least that was a reasoning given on stackoverflow. I mean, sure, one could easily implement a custom breakable
but that would take time.
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.
Scala prefers to encourage functional programming whenever possible :)
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.
That makes perfect sense then ;-)
val space = "( *)".r | ||
var str = regex.replaceAllIn(formalInput, "") | ||
var tokens = str.split(" ").toSeq | ||
var translation = "" // translation result |
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.
Perhaps a more idiomatic way would be to use a scala.collection.mutable.Buffer
, along with exceptions for short-circuiting.
def extractString(regex : scala.util.matching.Regex, matchedStr : String) : (String, Boolean) = { | ||
var resultStr = "" | ||
var errorCode = false | ||
try { | ||
resultStr = regex.findFirstIn(matchedStr).get | ||
} catch { | ||
case error:NoSuchElementException => errorCode = true | ||
} | ||
return (resultStr, errorCode) | ||
} |
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.
A more idiomatic way to achieve that would be to reuse the Option
returned by the matcher:
def extractString(regex : scala.util.matching.Regex, matchedStr : String) : (String, Boolean) = { | |
var resultStr = "" | |
var errorCode = false | |
try { | |
resultStr = regex.findFirstIn(matchedStr).get | |
} catch { | |
case error:NoSuchElementException => errorCode = true | |
} | |
return (resultStr, errorCode) | |
} | |
def extractString(regex : scala.util.matching.Regex, matchedStr : String) : Option[String] = | |
regex.findFirstIn(matchedStr) |
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.
Oh, but then I keep getting what I didn't want, i.e., this whole function was about extracting the string from Some(string_I_want)
. And this change gives me the unwanted Some(string)
and None
in the final output...
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.
More precisely: that's what I get after changing the function:
Given Some(pel24_1): It is not the case that there exists X such that Some(big_s)and Some(big_q).
And that's what I have with the previous extractString()
:
Given pel24_1: It is not the case that there exists X such that big_s and big_q.
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.
well, if you know for sure that your option is nonempty, you can use myOption.get
. Otherwise, you can do something like
myOption match {
case Some(value) => value
case None => ""
}
Or even better: myOption.getOrElse("")
translation += extractString(propertyName, x)._1 + ' ' | ||
errorCode = extractString(propertyName, x)._2 |
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.
Then, e.g.
translation += extractString(propertyName, x)._1 + ' ' | |
errorCode = extractString(propertyName, x)._2 | |
extractString(propertyName, x) match { | |
case Some(s) => translation += s | |
case None => errorCode = true | |
} |
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.
I see, thanks!
*/ | ||
def aceToTptp(text: String) : String = { | ||
var query = text.replaceAll("\\s", "+") | ||
return get(s"http://attempto.ifi.uzh.ch/ws/ape/apews.perl?text=$query&ctptp=on&solo=tptp") |
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.
The return
keyword is optional when the expression is the last one in the block; it's a good practice to omit it when possible.
Many thanks for all the suggestions! That's exactly the sort of feedback I was hoping for because I'm not familiar with idiomatic solutions :-) |
If you need or want it, at some point, We can have a zoom meeting and I can show you idiomatic scala practice and usefull tips for your code. |
That would be really helpful, thank you! I don't want to take too much of your valuable time but I do need some pointers so that I don't make too many stylistic/idiomatic mistakes in the future. |