You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "iamsanjay (via GitHub)" <gi...@apache.org> on 2024/03/22 07:14:53 UTC
[PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
iamsanjay opened a new pull request, #13198:
URL: https://github.com/apache/lucene/pull/13198
<!--
If this is your first contribution to Lucene, please make sure you have reviewed the contribution guide.
https://github.com/apache/lucene/blob/main/CONTRIBUTING.md
-->
Fix #13191
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "iamsanjay (via GitHub)" <gi...@apache.org>.
iamsanjay commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2020515624
@dweiss Thanks for the clarification, It does change the seed and hence was not able to reproduce the failure case. To increase the likelihood I switch to choosing only from two values (0,1), instead of `random.nextFloat()` and able to reproduce it without any Hassle. So far the issue is with the test code which is producing the expected aggregated Facet Results and only includingpositive values.
@stefanvodita You are right! #12966 would consider the non-positive values as well, and the change that I introduced would start failing.
So I revert the change that I did for aggregation and ran the boundary test (including negative 1) case, and It failed again. Try to include this new boundary case and you will see it.
```
public void testBoundaryCases() throws Exception {
final float[] boundaryCases = new float[] {-1, 0, 1};
doTestDifferentValueRanges(() -> boundaryCases[random().nextInt(boundaryCases.length)]);
}
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2014855713
Thanks for working on this. Once #12966 is merged, solving #12585, we would want to revert this change, right?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "iamsanjay (via GitHub)" <gi...@apache.org>.
iamsanjay commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2046550816
> @iamsanjay - thank you for working on this! I merged #12966, which should mean the original test failure is fixed. Do you want to verify that all is working as expected now?
I tried running below test case which will only test two values [0,1], and it's failing.
```
public void testBoundaryCases() throws Exception {
final float[] boundaryCases = new float[] {0, 1};
doTestDifferentValueRanges(() -> boundaryCases[random().nextInt(boundaryCases.length)]);
}
```
You can also use this pull request, only remove the fix in check result method, to reproduce it. The idea is to check for only 0 and 1. Because in existing case we are getting values from random.nextFloat which makes it almost impossible to generate zeroes or ones.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2015144997
FYI, the test is fixed by doing the following instead:
```
--- a/lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestTaxonomyFacetValueSource.java
+++ b/lucene/facet/src/test/org/apache/lucene/facet/taxonomy/TestTaxonomyFacetValueSource.java
@@ -53,6 +53,7 @@ import org.apache.lucene.search.TopDocs;
import org.apache.lucene.store.Directory;
import org.apache.lucene.tests.analysis.MockAnalyzer;
import org.apache.lucene.tests.index.RandomIndexWriter;
+import org.apache.lucene.tests.util.LuceneTestCase;
import org.apache.lucene.tests.util.TestUtil;
import org.apache.lucene.util.IOUtils;
@@ -597,7 +598,12 @@ public class TestTaxonomyFacetValueSource extends FacetTestCase {
Directory indexDir = newDirectory();
Directory taxoDir = newDirectory();
- RandomIndexWriter w = new RandomIndexWriter(random(), indexDir);
+ RandomIndexWriter w =
+ new RandomIndexWriter(
+ random(),
+ indexDir,
+ LuceneTestCase.newIndexWriterConfig()
+ .setMergePolicy(LuceneTestCase.newMergePolicy(random(), false)));
DirectoryTaxonomyWriter tw = new DirectoryTaxonomyWriter(taxoDir);
FacetsConfig config = new FacetsConfig();
int numDocs = atLeast(1000);
benjamintrent@Benjamins-MacBook-Pro-2:
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2047250772
Thank you for persisting @iamsanjay! I spend a bit of time on this and noticed two bugs, which should be fixed by #13287. Feel free to add the changes to your PR. I can still get some failures with your test, so I expect there's at least one other bug. Maybe you have better luck than I reproducing it with a smaller test.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2017652016
Whenever you touch the random number generator, it'll change anything down from there. Reiterate/Beast your tests to find a new offending seed (or improve the probability your change fixes the problem):
https://github.com/apache/lucene/blob/main/help/tests.txt#L89-L123
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2039599158
@iamsanjay - thank you for working on this! I merged #12966, which should mean the original test failure is fixed. Do you want to verify that all is working as expected now?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-3408725251
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "iamsanjay (via GitHub)" <gi...@apache.org>.
iamsanjay commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2015519505
@benwtrent I added above code and for some reason the Facet results have completely changed and did not include the children with value zero. Hence, it's passing. However, If i try with new boundary test case which I added it's failing. I am still digging into this more to see How changing `RandomIndexWriter` can help.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2076082854
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix TestTaxonomyFacetValueSource.testRandom [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #13198:
URL: https://github.com/apache/lucene/pull/13198#issuecomment-2017533598
Aren't we changing the random number generation when we add the merge policy, so we're no longer producing a failing case by chance?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org