Multi clause printf by hkir-dev · Pull Request #336 · INCATools/dosdp-tools

hkir-dev · 2021-06-15T15:47:48Z

In relation with the issue: INCATools/dead_simple_owl_design_patterns#71
and relatedly updated ontology schema, implementations to add multi-clause printf and internal_vars.

With this implementation:

internal_vars that enable manipulating vars (applying some functions upon) and introducing as new internal variables. Currently 'join' function is implemented that joins list_var or data_list_var elements with a specified separator.
multi_clause_printf that enable defining print format string as multiple interrelated sub-strings (clauses). Aim is to:
- Clauses to define optional parts of the print format string
- Define clause dependencies (sub-clauses) whose existence depends on the existence of the parent clause.

Backing usecase is obophenotype/brain_data_standards_ontologies#98

dosumis · 2021-06-16T14:14:37Z

@balhoff - would be great if you could take a look soon. Thanks!

balhoff · 2021-06-18T14:30:13Z

Thanks! I'll try to get it done. Awesome to get a contribution!

balhoff

@hkir-dev I just made a few quick style comments, before trying out the functionality. One general thing, please use camelCase variables everywhere, except specifically in DOSDP fields that get turned to JSON.

balhoff · 2021-06-18T14:34:04Z

src/main/scala/org/monarchinitiative/dosdp/DOSDP.scala

-  def replaced(text: String, vars: Option[List[String]], bindings: Option[Map[String, SingleValue]], quote: Boolean): Option[String] = {
+  def replaced(text: Option[String], vars: Option[List[String]], multi_clause: Option[MultiClausePrintf], bindings: Option[Map[String, SingleValue]], quote: Boolean): Option[String] = {
+    if(text.isDefined) {
+      return replace_text(text, vars, bindings, quote)


Never use return (https://tpolecat.github.io/2014/05/09/return.html).

All returns removed from code.

balhoff · 2021-06-18T14:40:58Z

src/main/scala/org/monarchinitiative/dosdp/DOSDP.scala

+    None
+  }
+
+  private def replace_text(text: Option[String], vars: Option[List[String]], bindings: Option[Map[String, SingleValue]], quote: Boolean) = {


It seems like text should be String, not Option[String] (handle missing text outside of this method). Also, I like to add type signatures to method definitions.

Based on the new schema text or multi_clause can be used to define printf formats.

oneOf: - required: [ text, vars ] - required: [ multi_clause ]

Text is not a mandatory field now, we can use multi_clause instead of text.

balhoff · 2021-06-18T14:42:45Z

src/main/scala/org/monarchinitiative/dosdp/DOSDP.scala

+    if(text.isDefined) {
+      return replace_text(text, vars, bindings, quote)
+    } else if (multi_clause.isDefined) {
+      val replaced_texts = for {
+        multi_clauses <- multi_clause.toSeq
+        printf_clauses <- multi_clauses.clauses.toSeq
+        printf_clause <- printf_clauses
+        printf_clause_text <- replaced(Some(printf_clause.text), printf_clause.vars, None, bindings, quote)
+        sub_clauses = printf_clause.sub_clauses.getOrElse(List.empty[MultiClausePrintf])
+        sub_clauses_texts = sub_clauses.flatMap(mc => replaced(None, None, Some(mc), bindings, quote))
+        clause_text = (printf_clause_text :: sub_clauses_texts).mkString(multi_clauses.sep.getOrElse(" "))
+      } yield clause_text
+      val result = replaced_texts.filter(_.nonEmpty).mkString(multi_clause.get.sep.getOrElse(" "))
+      val trimmed = result.trim
+      if (trimmed.isEmpty) return None else return Some(trimmed)
+    }
+    None
+  }


Maybe the optionality of text should be handled outside of this method, and you could break this into two separate methods? I would not use isDefined (they should remove it from the Option type); I prefer an expression-oriented style.

replaceMultiClause method extracted. isDefined bassed logic replaced with flatMap and list operations.

balhoff · 2021-06-18T14:44:46Z

src/main/scala/org/monarchinitiative/dosdp/DOSDP.scala

                            annotations: Option[List[Annotations]],
                            axiom_type: AxiomType,
-                            text: String,
+                            text: Option[String],


Was this changed in the spec? Just want to make sure!

Now, the printf format can be given through text or a multi_clause. Updated spec is here: https://github.com/INCATools/dead_simple_owl_design_patterns/blob/e5d69a6f3e17ec464e6942ccf0f05991694cd671/spec/DOSDP_schema_full.yaml#L135

balhoff · 2021-06-18T14:50:00Z

src/main/scala/org/monarchinitiative/dosdp/cli/Generate.scala

+      val internalVarBindings = (for {
+        internalVars <- dosdp.internal_vars.toSeq
+        internalVar <- internalVars
+        function <- internalVar.apply.toSeq
+        value = function.apply(Some(dataListBindings.getOrElse(internalVar.input, listVarBindings.getOrElse(internalVar.input, MultiValue(Set.empty[String])))))
+        if value.isDefined
+      } yield internalVar.var_name ->  SingleValue(value.get) ).toMap


Instead, avoid isDefined and get (I didn't test this):

val internalVarBindings = (for { internalVars <- dosdp.internal_vars.toSeq internalVar <- internalVars function <- internalVar.apply.toSeq value <- function.apply(Some(dataListBindings.getOrElse(internalVar.input, listVarBindings.getOrElse(internalVar.input, MultiValue(Set.empty[String]))))) } yield internalVar.var_name -> SingleValue(value) ).toMap

Recommended logic worked perfectly, changed accordingly.

balhoff · 2021-06-18T14:51:20Z

src/main/scala/org/monarchinitiative/dosdp/DOSDP.scala

+
+final case class JoinFunction(join: Join) extends Function {
+  override def apply(input_var:Option[Binding]): Option[String] = {
+    val joined_values = input_var.getOrElse(MultiValue(Set.empty[String])).asInstanceOf[MultiValue].value.mkString(join.sep)


Can this be written without casting (asInstanceOf)?

asInstanceOf based logic re-implemented with collect { case a: MultiValue => a }

balhoff · 2021-06-18T14:51:27Z

src/main/scala/org/monarchinitiative/dosdp/DOSDP.scala

+
+final case class RegexFunction(regex: RegexSub) extends Function {
+  override def apply(input_var:Option[Binding]): Option[String] = {
+    val applied_value = input_var.getOrElse(SingleValue("")).asInstanceOf[SingleValue].value


Can this be written without casting (asInstanceOf)?

asInstanceOf based logic re-implemented with collect { case a: SingleValue => a }

…pace

balhoff · 2021-07-01T17:42:41Z

Looks great, thank you for also writing tests!

hkir-dev added 3 commits June 10, 2021 21:59

#71 multi_clause support

d323cea

#71 multi_clause support

5fb0df4

#71 multi_clause support

ee1e025

hkir-dev requested review from balhoff and dosumis June 15, 2021 15:47

dosumis mentioned this pull request Jun 17, 2021

DOSDP tools update - support for multi-clause statements in string fns obophenotype/brain_data_standards_ontologies#111

Closed

balhoff requested changes Jun 18, 2021

View reviewed changes

hkir-dev added 2 commits June 21, 2021 12:37

#71 review fixes

1a56874

#71 bugfix and additional tests for multi_clause separators without s…

7574b86

…pace

hkir-dev requested a review from balhoff June 28, 2021 13:39

balhoff added 2 commits July 1, 2021 13:37

Applied formatting.

386072c

Use camelcase.

0a599d1

balhoff approved these changes Jul 1, 2021

View reviewed changes

balhoff merged commit f5e2050 into master Jul 1, 2021

balhoff deleted the multi_clause_printf branch July 1, 2021 17:42

dosumis mentioned this pull request Jul 19, 2021

Support 0-many var input #334

Closed

Conversation

hkir-dev commented Jun 15, 2021

Uh oh!

dosumis commented Jun 16, 2021

Uh oh!

balhoff commented Jun 18, 2021

Uh oh!

balhoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balhoff commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants