search

fozziethebeat / S-Space

The S-Space repsitory, from the AIrhead-Research group
GNU General Public License v2.0
205 stars 106 forks source link

Bug in SemanticSpaceIO.writeText #9

Closed davidjurgens closed 12 years ago

davidjurgens commented 12 years ago

When using a SemanticSpace with sparse vectors, SemanticSpaceIO.writeText() incorrectly saves the vectors in sparse format rather than dense format. This is due to calling VectorIO.toString(), which treats SparseVector instances specially.

The fix is to either remove the dependency on VectorIO so that all the values are written, or to update VectorIO to refactor the SparseVector functionality into a separate method (perhaps a SparseVector overload?).

In the current state, I don't think the .sspace files write in this hybrid format are readable, though it's amazing that we hadn't noticed that for so long.

fozziethebeat commented 12 years ago

This is very true. I don't think we or anyone really uses the full dense text format. Do you want me to fix it directly in the master or do you want to add in a fix to the graph update code?

davidjurgens commented 12 years ago

If we want to merge the graph updates branch today, I can patch it in there. Otherwise, I don't think I have access to write master at the moment to fix it there.

On Fri, Mar 2, 2012 at 10:14 AM, Keith Stevens < reply@reply.github.com

wrote:

This is very true. I don't think we or anyone really uses the full dense text format. Do you want me to fix it directly in the master or do you want to add in a fix to the graph update code?

Reply to this email directly or view it on GitHub: https://github.com/fozziethebeat/S-Space/issues/9#issuecomment-4290977

fozziethebeat commented 12 years ago

You can't write to master? Hmm, I should fix that. Let me figure out how to do that/why that is. In the mean time, can you put that fix into the graph branch and then we'll merge it this afternoon?

davidjurgens commented 12 years ago

Ok, the code is now in place. Just let me know when you want to start this merging business.

On Fri, Mar 2, 2012 at 10:27 AM, Keith Stevens < reply@reply.github.com

wrote:

You can't write to master? Hmm, I should fix that. Let me figure out how to do that/why that is. In the mean time, can you put that fix into the graph branch and then we'll merge it this afternoon?

Reply to this email directly or view it on GitHub: https://github.com/fozziethebeat/S-Space/issues/9#issuecomment-4291247

fozziethebeat commented 12 years ago

Awesome.

Although there's one compilation issue on my side with the graph branch. I get

[javac] /home/stevens35/devel/S-Space/src/edu/ucla/sspace/graph/isomorphism/TypedIsomorphicGraphCounter.java:177:    <E>pack(edu.ucla.sspace.graph.Graph<E>) in edu.ucla.sspace.graph.Graphs cannot be applied to (G)
[javac]         g = (G)(Graphs.pack(g));

Any idea on why that's happening?

davidjurgens commented 12 years ago

That's pretty odd. There's a comment above it saying that the compiler complains about the type (which can be safely ignored due to the covariant return type of pack()) but I never saw a compiler error. I've also compiled the code on linux and OS X. Let me try a fresh check out.... Also, just to be ultra-paranoid, what Java version are you using?

On Fri, Mar 2, 2012 at 11:42 AM, Keith Stevens < reply@reply.github.com

wrote:

Awesome.

Although there's one compilation issue on my side with the graph branch. I get

[javac] /home/stevens35/devel/S-Space/src/edu/ucla/sspace/graph/isomorphism/TypedIsomorphicGraphCounter.java:177:

pack(edu.ucla.sspace.graph.Graph) in edu.ucla.sspace.graph.Graphs cannot be applied to (G) [javac] g = (G)(Graphs.pack(g)); Any idea on why that's happening? --- Reply to this email directly or view it on GitHub: https://github.com/fozziethebeat/S-Space/issues/9#issuecomment-4292593
fozziethebeat commented 12 years ago

java version "1.6.0_17" Java(TM) SE Runtime Environment (build 1.6.0_17-b04) Java HotSpot(TM) 64-Bit Server VM (build 14.3-b01, mixed mode)

fozziethebeat commented 12 years ago

Scary news. It compiles just fine with java version "1.6.0_26"

davidjurgens commented 12 years ago

Huh. Looks like a bug in the VM got fixed at some point :) Is it fine if we leave the code as is then? I think the VM for JDK 6 is 1.6.0_31 at this point so most people will hopefully not see that compiler issue.

On Fri, Mar 2, 2012 at 1:57 PM, Keith Stevens < reply@reply.github.com

wrote:

Scary news. It compiles just fine with java version "1.6.0_26"

Reply to this email directly or view it on GitHub: https://github.com/fozziethebeat/S-Space/issues/9#issuecomment-4294831

fozziethebeat commented 12 years ago

Alrighty, branch merged. Most of the conflicts came up from moving the sim package to similarity. The others were minor things that git got confused about.