issue #121 test #122

draeger-lab · Aug 2, 2022 · ebb4f05 · ebb4f05
1 parent 931d760
commit ebb4f05
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 66 deletions.
diff --git a/src/main/java/edu/ucsd/sbrg/bigg/polishing/ReactionPolishing.java b/src/main/java/edu/ucsd/sbrg/bigg/polishing/ReactionPolishing.java
@@ -16,6 +16,7 @@
 import java.util.ResourceBundle;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 import static java.text.MessageFormat.format;
 
@@ -159,32 +160,29 @@ private void setSBOTermFromPattern(BiGGId id) {
    *         else {@link Optional#of}, where the wrapped string is the compartment code
    */
   private Optional<String> polish(ListOf<SpeciesReference> speciesReferences, int defaultSBOterm) {
-    Optional<String> compartmentId = Optional.empty();
-    Model model = speciesReferences.getModel();
+    // set defaults
     for (SpeciesReference sr : speciesReferences) {
       if (!sr.isSetSBOTerm() && !Parameters.get().omitGenericTerms()) {
         sr.setSBOTerm(defaultSBOterm);
       }
       if (!sr.isSetConstant()) {
         sr.setConstant(false);
       }
-      if (model == null) {
-        continue;
-      }
-      Species species = model.getSpecies(sr.getSpecies());
-      if (species != null) {
-        // Assumed intention here, that conflicting compartment information cannot be resolved
-        if (!species.isSetCompartment()
-          || compartmentId.map(id -> !id.equals(species.getCompartment())).orElse(false)) {
-          return compartmentId;
-        } else {
-          compartmentId = Optional.of(species.getCompartment());
-        }
-      } else {
-        logger.info(format(MESSAGES.getString("SPECIES_REFERENCE_INVALID"), sr.getSpecies()));
-      }
     }
-    return compartmentId;
+    // determine common compartment
+    Model model = speciesReferences.getModel();
+    if (null != model) {
+      var modelSpecies = speciesReferences.stream()
+              .map(SpeciesReference::getSpeciesInstance)
+              .map(Optional::ofNullable)
+              .map(o -> o.map(Species::getCompartmentInstance))
+              .flatMap(Optional::stream)
+              .map(Compartment::getId)
+              .collect(Collectors.toSet());
+
+      return modelSpecies.size() == 1 ? modelSpecies.stream().findFirst() : Optional.empty();
+    }
+    return Optional.empty();
   }
 
 

diff --git a/src/test/java/edu/ucsd/sbrg/bigg/polishing/ReactionPolishingTest.java b/src/test/java/edu/ucsd/sbrg/bigg/polishing/ReactionPolishingTest.java
@@ -157,51 +157,8 @@ public void conflictingProductsLeadToUnset() {
         assertEquals(null, r.getCompartmentInstance());
     }
 
-    /**
-     * Note that this test is to be understood in tandem with the next one.
-     * Together they show that not only are inconsistent compartments in reaction
-     * products handled fundamentally different that in reactants,
-     * there also is an ordering dependency at play there.
-     * <p>
-     * I considers this behaviour a bug, the unit tests are here to document this
-     * and verify that the fix once implemented works.
-     */
     @Test
-    public void orderingDependencyProductCompartmentsLeadToUnset1() {
-        var model = new Model(3, 2);
-        initParameters();
-        var r = model.createReaction("some_reaction");
-        var cytosol = model.createCompartment("c");
-        var extracellular = model.createCompartment("e");
-
-        r.setCompartment(cytosol);
-
-        var s1 = model.createSpecies("s1", cytosol);
-        r.createProduct(s1);
-
-        var s2 = model.createSpecies("s2", extracellular);
-        r.createProduct(s2);
-
-        assertEquals(cytosol, r.getCompartmentInstance());
-
-        new ReactionPolishing(r).polish();
-
-        assertEquals(2, r.getListOfProducts().size());
-        assertEquals(Set.of(cytosol, extracellular), r.getListOfProducts().stream()
-                .map(SpeciesReference::getSpeciesInstance)
-                .map(Species::getCompartmentInstance)
-                .collect(Collectors.toSet()));
-        assertEquals(cytosol, r.getCompartmentInstance());
-    }
-
-    /**
-     * Please take note of the comment on the test above, as these only make
-     * sense in tandem.
-     * <p>
-     * tl;dr: this test documents a bug
-     */
-    @Test
-    public void orderingDependencyProductCompartmentsLeadToUnset2() {
+    public void inconsistentProductCompartmentsLeadToUnset() {
         var model = new Model(3, 2);
         initParameters();
         var r = model.createReaction("some_reaction");
@@ -296,10 +253,11 @@ public void defaultsAreSetOnSpeciesReferences() {
     }
 
     /**
-     * Note: this test documents a bug.
+     * Note: this test documents
+     * https://github.com/draeger-lab/ModelPolisher/issues/122
      */
     @Test
-    public void defaultsAreAbortedMidway() {
+    public void defaultsAreSetEvenIfCompartmentsAreInconsistent() {
         var model = new Model(3, 2);
         initParameters();
         var r = model.createReaction("some_reaction");
@@ -327,10 +285,10 @@ public void defaultsAreAbortedMidway() {
         assertEquals(3, r.getListOfProducts().size());
         assertEquals("SBO:0000011", product1.getSBOTermID());
         assertEquals("SBO:0000011", product2.getSBOTermID());
-        assertEquals("", product3.getSBOTermID());
+        assertEquals("SBO:0000011", product3.getSBOTermID());
         assertTrue(product1.isSetConstant());
         assertTrue(product2.isSetConstant());
-        assertFalse(product3.isSetConstant());
+        assertTrue(product3.isSetConstant());
     }
 
     /**