[GitHub] [opennlp] eihli commented on pull request #392: OPENNLP-1330 Fix Parser's top "k" parses functionality from returning bottom "k" parses.

[GitBox <gi...@apache.org>]

eihli commented on pull request #392:
URL: https://github.com/apache/opennlp/pull/392#issuecomment-864057912


   I think it's OK to merge this without adding a test. I couldn't find a location in the existing tests files that would be a good spot to add this. I'd love to dig in and figure out how to make a test case for this fit within an existing test file or create a new test. But that's a tall order (for me, given my newness to the codebase). 
   
   It would add significant delay for a small change that can be confidently reasoned about. The risk, in this case, is not that a code-related bug is introduced. The risk is that a reason/understanding-based bug is introduced. A test wouldn't fix faulty reasoning.
   
   As for the reason this is `first` rather than `last`, https://docs.oracle.com/javase/7/docs/api/java/util/TreeSet.html.
   
   ```
   A NavigableSet implementation based on a TreeMap. 
   The elements are ordered using their natural ordering, 
   or by a Comparator provided at set creation time, 
   depending on which constructor is used. 
   ```
   
   `completeParses` is defined as a TreeSet on line 187:
   
   ```
   completeParses = new TreeSet<>();
   ```
   
   And `Parse`s that are added to `completeParses` have a compareTo so they are ordered by their log probability.
   
   ```
     public int compareTo(Parse p) {
       return Double.compare(p.getProb(), this.getProb());
     }
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org