Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error message suggests strange types #1863

Closed
vasily-kirichenko opened this issue Nov 27, 2016 · 24 comments
Closed

Error message suggests strange types #1863

vasily-kirichenko opened this issue Nov 27, 2016 · 24 comments

Comments

@vasily-kirichenko
Copy link
Contributor

image

In particular, I don't see how Byte can fit here.

/cc @forki

@forki
Copy link
Contributor

forki commented Nov 27, 2016

It suggests the correct type as first thing. I think it's ok

@forki
Copy link
Contributor

forki commented Nov 27, 2016

I would have DateTimeKind expected to be on second position. But I assume we would need a different distance function for that. Also we could filter better so that Byte and Double would be eliminated. Do you have a heuristic in mind?

@forki
Copy link
Contributor

forki commented Nov 28, 2016

I remember @Rickasaurus proposed to use Jaro-Winkler Distance instead of Damerau-Levenstein edit distance. Quick google search showed Rick already blogged an implementation: http://richardminerich.com/2011/09/record-linkage-algorithms-in-f-jaro-winkler-distance-part-1/

So I copied it and tested on our sample:

image

Pretty neat. @Rickasaurus are you interested in contributing the algorithm? I would take care about the rest,

@forki
Copy link
Contributor

forki commented Nov 28, 2016

see #1876

@nosami
Copy link
Contributor

nosami commented Nov 28, 2016

screen shot 2016-11-28 at 16 03 22

First time I've seen this - awesome!

@forki
Copy link
Contributor

forki commented Nov 28, 2016

@nosami one day we want that to provide quickfixes ;-)

@Rickasaurus
Copy link
Contributor

Rickasaurus commented Nov 28, 2016

Sure, just grab it. In practice we double down on the first two letters even harder by doubling up the winkler coefficient, but I'm not completely sure you'd want to do that here. We use the scoring values for more than just ordering results.

@forki
Copy link
Contributor

forki commented Nov 28, 2016

@Rickasaurus unfortunatly this is not so easy with this repo. CLA, lawyers and stuff. But I would prepare #1876 - so that in the end you would just need to resent the PR with your CLA. Deal? And thanks for helping

@Rickasaurus
Copy link
Contributor

If that's the case I'll need to check with the higher ups at my company who will certainly say yes to the submission, but I'm not sure about the CLA. We'll probably need our lawyers to look at it.

@forki
Copy link
Contributor

forki commented Nov 28, 2016

ok. if that's going to be a problem then just let me know. I'm pretty sure I can google that algorithm and implement it myself if that's really needed. But of course I'd like to credit your work here.

forki added a commit to forki/visualfsharp that referenced this issue Nov 28, 2016
@Rickasaurus
Copy link
Contributor

Sure, where can I find the CLA? I'll send it today.

@forki
Copy link
Contributor

forki commented Nov 28, 2016 via email

@cartermp
Copy link
Contributor

cartermp commented Nov 28, 2016

@Rickasaurus when you make a PR a bot should comment and give you the info to sign the CLA - it uses GitHub authentication and after you've signed it, it'll mark the PR as already-signed and you'll be good to go from then. You can also look at it here: https://cla.microsoft.com/cladoc/microsoft-contribution-license-agreement.pdf

It's quite straightforward and only requires a few things like your soul that you have permission to make contributions by your employer if you're doing it for your employer.

@Rickasaurus
Copy link
Contributor

Rickasaurus commented Nov 28, 2016

Thanks for the link. I sent an email to our CEO about it. It's a bit murky because we have a faster internal implementation we could share instead. I'll come back when I know more.

@Rickasaurus
Copy link
Contributor

I've been given the go ahead to contribute this as Bayard Rock work. If I submit as myself will the proper corporate CLA be given?

@cartermp
Copy link
Contributor

cc @martinwoodward to answer this question. I suspect there's no issue, but Martin could clarify.

@martinwoodward
Copy link
Member

So section 4 of the CLA is the important bit. Basically, if the PR is made in the course of your work for an employer or your employer has intellectual property rights in the PR, you must have got permissions from them before signing the agreement. How you get permission varies from company to company - but if you are happy that they are happy for this to be contributed then you can sign your CLA and you are saying that you are allowed to contribute the IP as open source. Hope that makes sense - but please make sure you have taken a look at Section 4 before signing.

@Rickasaurus
Copy link
Contributor

Thanks Martin. We've already had our lawyer look at it and I have the go ahead from our CEO.

@martinwoodward
Copy link
Member

Fantastic news - thanks for doing that. Sounds like you'll be good to go. ✨

@Rickasaurus
Copy link
Contributor

So what's the next step?

@cartermp
Copy link
Contributor

@Rickasaurus In this case, all you gotta do is make your first PR. A bot will ask you to sign the CLA (logged in with credentials), and that'll be that. If you've already signed, it'll mark the PR with the cla-already-signed label, and from then on it's just code review time.

@forki
Copy link
Contributor

forki commented Nov 28, 2016 via email

forki added a commit to forki/visualfsharp that referenced this issue Nov 29, 2016
forki added a commit to forki/visualfsharp that referenced this issue Nov 29, 2016
forki added a commit to forki/visualfsharp that referenced this issue Nov 29, 2016
@forki
Copy link
Contributor

forki commented Nov 29, 2016

@Rickasaurus Ok I think #1876 will be green now. So you could just resubmit 6b48059 as a new PR. I will then close mine in favour of yours.

@forki
Copy link
Contributor

forki commented Nov 30, 2016

forki added a commit to forki/visualfsharp that referenced this issue Dec 1, 2016
forki added a commit to forki/visualfsharp that referenced this issue Dec 2, 2016
forki added a commit to forki/visualfsharp that referenced this issue Dec 4, 2016
forki added a commit to forki/visualfsharp that referenced this issue Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants