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

Ported over Lucene.Net.Spatial #174

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@nazjunaid
Contributor

nazjunaid commented Jul 26, 2016

I ported over Lucene.Net.Spatial using https://github.com/apache/lucene-solr/tree/releases/lucene-solr/4.8.1/lucene/spatial as reference

Let me know what you think

@synhershko

This comment has been minimized.

Show comment
Hide comment
@synhershko

synhershko Aug 2, 2016

Contributor

Looks good! will review shortly, thanks!

Contributor

synhershko commented Aug 2, 2016

Looks good! will review shortly, thanks!

@synhershko

This comment has been minimized.

Show comment
Hide comment
@synhershko

synhershko Aug 6, 2016

Contributor

Is this a full and complete port (e.g. no classes or functionality left behind)? if so, how did you manage to avoid updating spatial4n to bring it up to date with the latest spatial4j?

Contributor

synhershko commented Aug 6, 2016

Is this a full and complete port (e.g. no classes or functionality left behind)? if so, how did you manage to avoid updating spatial4n to bring it up to date with the latest spatial4j?

@nazjunaid

This comment has been minimized.

Show comment
Hide comment
@nazjunaid

nazjunaid Aug 13, 2016

Contributor

Sorry I didn't think about porting spatial4j. Looks like Lucene 4.8.1 is targeting Spatial4j 0.4.1. The only class I had trouble with porting was SerializedDVStrategy, it's reliant on BinaryCodec from spatial4j which I wasn't able to resolve. We probably need to port spatial4j i'll try and see if I can port it

Contributor

nazjunaid commented Aug 13, 2016

Sorry I didn't think about porting spatial4j. Looks like Lucene 4.8.1 is targeting Spatial4j 0.4.1. The only class I had trouble with porting was SerializedDVStrategy, it's reliant on BinaryCodec from spatial4j which I wasn't able to resolve. We probably need to port spatial4j i'll try and see if I can port it

@synhershko

This comment has been minimized.

Show comment
Hide comment
@synhershko

synhershko Aug 15, 2016

Contributor

It's already ported, just not up to date : https://github.com/synhershko/spatial4n

Contributor

synhershko commented Aug 15, 2016

It's already ported, just not up to date : https://github.com/synhershko/spatial4n

@NightOwl888

This comment has been minimized.

Show comment
Hide comment
@NightOwl888

NightOwl888 Oct 4, 2016

Contributor

@nazjunaid, @synhershko, what is the status of this? Are either of you working on updating Spatial4j to 0.4.1?

Do either of you know whether the 2 dependencies of Spatial4n (GeoAPI 1.7.4, and NetTopologySuite 1.13.3) are up to date enough for Spatial4n 0.4.1? It doesn't seem like there is much work to be done to update Spatial4n itself.

Contributor

NightOwl888 commented Oct 4, 2016

@nazjunaid, @synhershko, what is the status of this? Are either of you working on updating Spatial4j to 0.4.1?

Do either of you know whether the 2 dependencies of Spatial4n (GeoAPI 1.7.4, and NetTopologySuite 1.13.3) are up to date enough for Spatial4n 0.4.1? It doesn't seem like there is much work to be done to update Spatial4n itself.

@eladmarg

This comment has been minimized.

Show comment
Hide comment
@eladmarg

eladmarg Oct 4, 2016

Contributor

Hi, what should be done here?
I can try to re-port the spatial4j will this solve the issue?

Contributor

eladmarg commented Oct 4, 2016

Hi, what should be done here?
I can try to re-port the spatial4j will this solve the issue?

@eladmarg

This comment has been minimized.

Show comment
Hide comment
@eladmarg

eladmarg Oct 4, 2016

Contributor

@nazjunaid , where are the test files? what have been ported? what's missing?

Contributor

eladmarg commented Oct 4, 2016

@nazjunaid , where are the test files? what have been ported? what's missing?

@synhershko

This comment has been minimized.

Show comment
Hide comment
@synhershko

synhershko Oct 4, 2016

Contributor

The spatial lib needs to be brought up to date with the Java 4.8.0 branch (which I believe this is what this PR does, but this needs verification). The latest spatial bits will not work before spatial4n (https://github.com/synhershko/Spatial4N) is brought up to speed with the same version the 4.8.0 version is using. I haven't looked into it's dependencies (GeoAPI and NTS) but there might be some work there. Heads up: this will bring up the strong naming discussion.

Contributor

synhershko commented Oct 4, 2016

The spatial lib needs to be brought up to date with the Java 4.8.0 branch (which I believe this is what this PR does, but this needs verification). The latest spatial bits will not work before spatial4n (https://github.com/synhershko/Spatial4N) is brought up to speed with the same version the 4.8.0 version is using. I haven't looked into it's dependencies (GeoAPI and NTS) but there might be some work there. Heads up: this will bring up the strong naming discussion.

@NightOwl888

This comment has been minimized.

Show comment
Hide comment
@NightOwl888

NightOwl888 Oct 5, 2016

Contributor

@eladmarg - Since an older version of spatial4n already exists at https://github.com/synhershko/spatial4n, it would be much less work to upgrade it than to port anew. If you have a text comparison tool like Beyond Compare (a free trial is available here(http://download.cnet.com/Beyond-Compare/3000-2242_4-10015731.html)) and some help from Git, you can easily determine the lines that have changed between the 2 versions and only port those lines into spatial4n.

I used the following procedure to upgrade BoboBrowse.Net. A project that took 3 weeks to port, took only 2 days to upgrade.

  1. Clone the spatial4j repo (note it helps to name the folder the version that you will checkout).
  2. Copy the whole spatial4j repo and all of its folders into another folder (note it helps to name the folder the version that you will checkout).
  3. In one copy, checkout the original branch (in Java) that this is based on.
  4. In the other, check out the branch that we are porting to.
  5. Using explorer, find all of the files that no longer exist in the new version and in the .NET version, move them into a folder outside of the repository folder (they may come in handy for code that moved from one class to another).
  6. Using explorer, right click on a file in the old Java version, and click "select left file for compare".
  7. Using explorer, go to the folder with the updated Java version and on the same file, right click and select "compare to <filename>".
  8. Filter out all of the unimportant changes (line breaks and comments) in Beyond Compare and port only the diff of the two into the corresponding file in spatial4n.
  9. Remove the unimportant differences filter, and choose "*" to view the entire file and go back through to get the latest documentation comments and change them in spatial4n accordingly.
  10. Repeat 6-9 for each file.
  11. In explorer, locate all of the new files that didn't exist in the old version and port them to spatial4n.

Let us know if you will be working on this - I was thinking about taking it up after working on the failing tests in Lucene.Net and missing tests in Lucene.Net.Codecs.

@synhershko - You should also seriously consider using this approach to get to the next version of Lucene.Net after 4.8.0 is released. It might shave a couple of years off of the process :).

Contributor

NightOwl888 commented Oct 5, 2016

@eladmarg - Since an older version of spatial4n already exists at https://github.com/synhershko/spatial4n, it would be much less work to upgrade it than to port anew. If you have a text comparison tool like Beyond Compare (a free trial is available here(http://download.cnet.com/Beyond-Compare/3000-2242_4-10015731.html)) and some help from Git, you can easily determine the lines that have changed between the 2 versions and only port those lines into spatial4n.

I used the following procedure to upgrade BoboBrowse.Net. A project that took 3 weeks to port, took only 2 days to upgrade.

  1. Clone the spatial4j repo (note it helps to name the folder the version that you will checkout).
  2. Copy the whole spatial4j repo and all of its folders into another folder (note it helps to name the folder the version that you will checkout).
  3. In one copy, checkout the original branch (in Java) that this is based on.
  4. In the other, check out the branch that we are porting to.
  5. Using explorer, find all of the files that no longer exist in the new version and in the .NET version, move them into a folder outside of the repository folder (they may come in handy for code that moved from one class to another).
  6. Using explorer, right click on a file in the old Java version, and click "select left file for compare".
  7. Using explorer, go to the folder with the updated Java version and on the same file, right click and select "compare to <filename>".
  8. Filter out all of the unimportant changes (line breaks and comments) in Beyond Compare and port only the diff of the two into the corresponding file in spatial4n.
  9. Remove the unimportant differences filter, and choose "*" to view the entire file and go back through to get the latest documentation comments and change them in spatial4n accordingly.
  10. Repeat 6-9 for each file.
  11. In explorer, locate all of the new files that didn't exist in the old version and port them to spatial4n.

Let us know if you will be working on this - I was thinking about taking it up after working on the failing tests in Lucene.Net and missing tests in Lucene.Net.Codecs.

@synhershko - You should also seriously consider using this approach to get to the next version of Lucene.Net after 4.8.0 is released. It might shave a couple of years off of the process :).

@eladmarg

This comment has been minimized.

Show comment
Hide comment
@eladmarg

eladmarg Oct 5, 2016

Contributor

I think I will write a tool for that, this will be good also for the next version of port.

@conniey, is it possible to ask the guys from TFS if they have the diff API? they already handled this kind of merging stuff. maybe they can provide this API with a couple of hours.

the logic is pretty simple,

  1. diff between the java versions
  2. then take each diff part and try to merge it to the C# version in the correct place / file.

this can be also added as comment with TODO label,
and even after the auto conversion tool (java2cs) of the code snippet.

I'm currently packed, so I'll try to get into it during the weekend.

Contributor

eladmarg commented Oct 5, 2016

I think I will write a tool for that, this will be good also for the next version of port.

@conniey, is it possible to ask the guys from TFS if they have the diff API? they already handled this kind of merging stuff. maybe they can provide this API with a couple of hours.

the logic is pretty simple,

  1. diff between the java versions
  2. then take each diff part and try to merge it to the C# version in the correct place / file.

this can be also added as comment with TODO label,
and even after the auto conversion tool (java2cs) of the code snippet.

I'm currently packed, so I'll try to get into it during the weekend.

@nazjunaid

This comment has been minimized.

Show comment
Hide comment
@nazjunaid

nazjunaid Oct 5, 2016

Contributor

@NightOwl888 I havn't started porting Spatial4j so feel free to do so

I started porting the tests for Lucene.Net.Spatial but was blocked on Spatial4j

Contributor

nazjunaid commented Oct 5, 2016

@NightOwl888 I havn't started porting Spatial4j so feel free to do so

I started porting the tests for Lucene.Net.Spatial but was blocked on Spatial4j

@NightOwl888

This comment has been minimized.

Show comment
Hide comment
@NightOwl888

NightOwl888 Oct 5, 2016

Contributor

@eladmarg

There are a couple of snags that could make that idea difficult:

  1. Whitespace differences (different number of tabs/spaces, different line break chars, etc)
  2. Different code formatting

Beyond Compare is great because it gives you the ability to ignore whitespace and even code comments. I am not sure if it has an API that can be utilized, though.

Let's say version 1 in Java has this function signature:

public int Foo(string arg1, string arg2, string arg3, string arg4, string arg5, string arg6, string arg7)
{
    return doSomething(string arg1, string arg2, string arg3, string arg4, string arg5, string arg6, string arg7);
}

Then for version 2, someone decided the code format should change to:

public int Foo(string arg1, string arg2, 
    string arg3, string arg4, 
    string arg5, string arg6, string arg7)
{
    return doSomething(string arg1, string arg2, 
        string arg3, string arg4, 
        string arg5, string arg6, string arg7);
}

If your diff tool isn't smart enough to understand code formatting, it will see this as a new function even though it is functionally equivalent.

Most diff tools are only smart enough to work line by line. That is why I like Beyond Compare because it can at least tell you which characters on the line changed and ignore trivial things such as tabs and spaces. Unfortunately, it isn't smart enough to detect code formatting changes, though (which would have cut about half of the time off of the BoboBrowse.Net port) - or maybe it is and I haven't found the setting for it... The main issue there was that someone decided that all of the opening curly braces { should be on the same line as the function header instead of the line after.

I am not saying this is something that can't (or shouldn't) be automated, but to ensure it works properly you should make the tool smart enough to know when two different formats of the same Java code are functionally identical - otherwise you have copied a lot of useless code into the .NET part that will need a lot of manual cleanup.

Of course, then there is also the matter of having the tool figure out where in the .NET code it should insert the function, since we will have completely different file headers and the functions will probably have different bodies, possibly different method accessibility, slightly different parameter types, different code formatting, possibly different function names, Java getters and setters that were made into .NET properties, etc.

If it just spit out the diffs into a new directory of files that could then be manually copied/ported into the .NET part you would have the best balance of automation vs manual cleanup. Maybe you could go a step further and automate the process of removing the files from .NET (and backing them up somewhere) that have been deleted in Java and creating new .cs files for the corresponding new files in Java (and maybe even porting the code using one of the existing automated porting tools).

@nazjunaid - thanks for the update.

Contributor

NightOwl888 commented Oct 5, 2016

@eladmarg

There are a couple of snags that could make that idea difficult:

  1. Whitespace differences (different number of tabs/spaces, different line break chars, etc)
  2. Different code formatting

Beyond Compare is great because it gives you the ability to ignore whitespace and even code comments. I am not sure if it has an API that can be utilized, though.

Let's say version 1 in Java has this function signature:

public int Foo(string arg1, string arg2, string arg3, string arg4, string arg5, string arg6, string arg7)
{
    return doSomething(string arg1, string arg2, string arg3, string arg4, string arg5, string arg6, string arg7);
}

Then for version 2, someone decided the code format should change to:

public int Foo(string arg1, string arg2, 
    string arg3, string arg4, 
    string arg5, string arg6, string arg7)
{
    return doSomething(string arg1, string arg2, 
        string arg3, string arg4, 
        string arg5, string arg6, string arg7);
}

If your diff tool isn't smart enough to understand code formatting, it will see this as a new function even though it is functionally equivalent.

Most diff tools are only smart enough to work line by line. That is why I like Beyond Compare because it can at least tell you which characters on the line changed and ignore trivial things such as tabs and spaces. Unfortunately, it isn't smart enough to detect code formatting changes, though (which would have cut about half of the time off of the BoboBrowse.Net port) - or maybe it is and I haven't found the setting for it... The main issue there was that someone decided that all of the opening curly braces { should be on the same line as the function header instead of the line after.

I am not saying this is something that can't (or shouldn't) be automated, but to ensure it works properly you should make the tool smart enough to know when two different formats of the same Java code are functionally identical - otherwise you have copied a lot of useless code into the .NET part that will need a lot of manual cleanup.

Of course, then there is also the matter of having the tool figure out where in the .NET code it should insert the function, since we will have completely different file headers and the functions will probably have different bodies, possibly different method accessibility, slightly different parameter types, different code formatting, possibly different function names, Java getters and setters that were made into .NET properties, etc.

If it just spit out the diffs into a new directory of files that could then be manually copied/ported into the .NET part you would have the best balance of automation vs manual cleanup. Maybe you could go a step further and automate the process of removing the files from .NET (and backing them up somewhere) that have been deleted in Java and creating new .cs files for the corresponding new files in Java (and maybe even porting the code using one of the existing automated porting tools).

@nazjunaid - thanks for the update.

@NightOwl888

This comment has been minimized.

Show comment
Hide comment
@NightOwl888

NightOwl888 Nov 7, 2016

Contributor

If there are no objections (meaning nobody else beat me to it), I would like to begin working on this.

Observations:

  • Spatial4n has dependencies on GeoAPI 1.7.2, and NetTopologySuite 1.13.2. The latest versions are 1.7.4 and 1.14.0. It doesn't look like Spatial4n uses these dependencies heavily, nor are there corresponding dependencies in Spatial4j, so will try the latest versions and see how it goes.
  • This was ported over with no tests.
Contributor

NightOwl888 commented Nov 7, 2016

If there are no objections (meaning nobody else beat me to it), I would like to begin working on this.

Observations:

  • Spatial4n has dependencies on GeoAPI 1.7.2, and NetTopologySuite 1.13.2. The latest versions are 1.7.4 and 1.14.0. It doesn't look like Spatial4n uses these dependencies heavily, nor are there corresponding dependencies in Spatial4j, so will try the latest versions and see how it goes.
  • This was ported over with no tests.
@eladmarg

This comment has been minimized.

Show comment
Hide comment
@eladmarg

eladmarg Nov 7, 2016

Contributor

Much to my regret I'm currently packed, so go ahead

Many thanks

Contributor

eladmarg commented Nov 7, 2016

Much to my regret I'm currently packed, so go ahead

Many thanks

@synhershko

This comment has been minimized.

Show comment
Hide comment
@synhershko

synhershko Nov 10, 2016

Contributor

@NightOwl888 go ahead. Spatial4n will be a bit challenging due to strong naming and possibly dependencies not being up-to-date. Let's take it there.

Contributor

synhershko commented Nov 10, 2016

@NightOwl888 go ahead. Spatial4n will be a bit challenging due to strong naming and possibly dependencies not being up-to-date. Let's take it there.

@NightOwl888

This comment has been minimized.

Show comment
Hide comment
@NightOwl888

NightOwl888 Nov 25, 2016

Contributor

Spatial is now complete.

The Spatial4n.Core and Spatial4n.Core.NTS dependent NuGet packages are currently on this MyGet feed: https://www.myget.org/F/spatial4n/api/v2

Contributor

NightOwl888 commented Nov 25, 2016

Spatial is now complete.

The Spatial4n.Core and Spatial4n.Core.NTS dependent NuGet packages are currently on this MyGet feed: https://www.myget.org/F/spatial4n/api/v2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment