You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2021/02/10 17:32:28 UTC
[GitHub] [datasketches-java] davecromberge opened a new pull request #349: Add jaccard similarity capability to Tuple sketches
davecromberge opened a new pull request #349:
URL: https://github.com/apache/datasketches-java/pull/349
This provides an analog to the current Theta sketch Jaccard Similarity
measure, but for Tuple sketches. The new similarity measure only
compares the hash table entries, but not the summary values themselves.
Note: This has been discussed on the dev mailing list, and is a draft for consideration. I am unsure as to whether this functionality is useful in a broader context, but the capability to compare the similarity/dissimilarity between two tuple sketches or between a tuple/theta sketch is absent.
Also, I have not run the linting/style formatters on this branch, and I would be grateful for help should this be an issue.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on pull request #349: Add jaccard similarity capability to Tuple sketches
Posted by GitBox <gi...@apache.org>.
leerho commented on pull request #349:
URL: https://github.com/apache/datasketches-java/pull/349#issuecomment-784422670
I will have to look at the checkstyle issues later and consider it for a subsequent PR.
Otherwise this PR looks good.
Thank you for submitting!
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] davecromberge commented on pull request #349: Add jaccard similarity capability to Tuple sketches
Posted by GitBox <gi...@apache.org>.
davecromberge commented on pull request #349:
URL: https://github.com/apache/datasketches-java/pull/349#issuecomment-783701140
Thank you for taking the time to go through the branch and make the code style changes @leerho.
I think that the use of "final" to prevent inadvertent bugs is a sensible policy, together with the other changes that you have made such as line length. To ensure that I prevent this in a future submission, I've configured my IDE to use the checkstyle plugin together with the `tools/SketchesCheckstyle.xml` configuration file. This seems to enforce the style rules that you have referred to, such as the 110-character line limit.
I've also spent some time trying to configure the build POM file to enable a user to run `mvn checkstyle:checkstyle` for the same purpose. However, it required an additional plugin with upgrade similar to this:
```
<build>
<plugins>
....
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>3.1.2</version>
<dependencies>
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>8.40</version>
</dependency>
</dependencies>
<configuration>
<configLocation>${project.basedir}/tools/SketchesCheckstyle.xml</configLocation>
</configuration>
</plugin>
```
I was unsuccessful at getting this to work, but I'm not sure how useful it would be given that there is an IDE option. On our projects, we enable a style checker on building a PR, but I think that you have configured similar checks already.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on pull request #349: Add jaccard similarity capability to Tuple sketches
Posted by GitBox <gi...@apache.org>.
leerho commented on pull request #349:
URL: https://github.com/apache/datasketches-java/pull/349#issuecomment-783589980
I made a number of code style changes reflected above. Nearly all of these can be caught by your IDE if properly configured. I have my line-length set to 110. We require "final" to be specified everywhere it is relevant including method arguments. Although some consider these to be noise, we have actually caught inadvertent bugs with this habit. The javadocs for generic methods should include @param tags for the generic parameters. I would do a compare of what I just submitted vs your original PR to see what was changed.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] davecromberge commented on pull request #349: Add jaccard similarity capability to Tuple sketches
Posted by GitBox <gi...@apache.org>.
davecromberge commented on pull request #349:
URL: https://github.com/apache/datasketches-java/pull/349#issuecomment-783702079
I'll reopen this for review 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho commented on pull request #349: Add jaccard similarity capability to Tuple sketches
Posted by GitBox <gi...@apache.org>.
leerho commented on pull request #349:
URL: https://github.com/apache/datasketches-java/pull/349#issuecomment-776892039
We are in the middle of a major release to 2.0.0 right now, so I will have to look at this in that context after the release.
Nonetheless, this is a good suggestion and thank you for the PR.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho merged pull request #349: Add jaccard similarity capability to Tuple sketches
Posted by GitBox <gi...@apache.org>.
leerho merged pull request #349:
URL: https://github.com/apache/datasketches-java/pull/349
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-java] leerho edited a comment on pull request #349: Add jaccard similarity capability to Tuple sketches
Posted by GitBox <gi...@apache.org>.
leerho edited a comment on pull request #349:
URL: https://github.com/apache/datasketches-java/pull/349#issuecomment-783589980
I made a number of code style changes reflected above. Nearly all of these can be caught by your IDE if properly configured. I have my line-length set to 110. We require "final" to be specified everywhere it is relevant including method arguments. Although some consider these to be noise, we have actually caught inadvertent bugs with this habit. The javadocs for generic methods should include "at" param tags for the generic parameters. I would do a compare of what I just submitted vs your original PR to see what was changed.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org