On 21/05/2009, Luc Maisonobe <luc.maison...@free.fr> wrote: > sebb a écrit : > > > On 21/05/2009, Luc Maisonobe <luc.maison...@free.fr> wrote: > >> Sam Halliday a écrit : > >> > >>> Bill, I've had a look at some of the recent changes... some comments, > >> > including some new thoughts:- > >> > > >> > - changing the return type to be actual classes was only supposed to > be for > >> > copy() for 2.0. Doing this on multiply, add, etc is a big mistake... > there > >> > is no guarantee that is the best thing to do and it is likely this will > >> > change in the future. This stands for all implementations. Great > thought > >> > needs to go into each method, not just by reading the current > >> > implementation. > >> > >> > >> It may help the compiler using directly the optimized versions in > >> statements like: > >> > >> m1.add(m2.multiply(m3.subtract(m4)) > >> > >> > >> > > >> > - I *strongly* urge you to remove Serializable from everything! > Please, we > >> > did this in MTJ and it turned out to be a major pain. A more > appropriate > >> > approach is to define a class for reading/writing Matrix Market files. > This > >> > can be a new feature in 2.1. If you're going to leave it, at least > document > >> > that the Serializable form is not guaranteed to remain compatible > across > >> > versions. > >> > >> > >> I disagree with and I think its me who started adding this interface > >> everywhere. The rationale is explained in the following use case: > >> > >> As a user, I want to implement an interface for some xyz library where > >> the interface already extends Serializable. In my implementation, I want > >> to have an instance field that is some RealMatrix. If RealMatrix does > >> not declare it extends Serializable, I keep getting warnings about > >> having a non-serializable field in a serializable class. Looking the > >> other way round having serializable and not using it never caused me any > >> pain because it was really a generalized pattern. Mixing serializable > >> and non serializable is difficult, putting it everywhere and sticking to > >> this policy is simple. > > > > One can use the "transient" marker for non-serializable fields. > > > Transient is OK when the field state can be reconstructed on > deserialization from other fields information. This is quite rare and is > surely not the case with matrices which may be huge and therefore not > duplicated in several different fields. > > > > > > Merely implementing Serializable will prevent the warnings, however in > > general it's not trivial to ensure that serialization works correctly. > > > This is probably simpler in math objects (except graph theory perhaps) > than in other business domains. Our objects typically hold a primitive > and arrays and some other lower level objects with similar simple > structures. >
Nevertheless, adding serialization needs to be considered carefully, as pointed out here: http://www.javapractices.com/topic/TopicAction.do?Id=45 Furthermore, readObject() cannot be used with final fields. Bloch (Effective Java) suggests using readResolve() instead, but even this has limitations. As the book points out, merely making a class Serializable is equivalent to adding a public constructor which sets all the non-transient fields without perfoming any validation. If there are any constraints on the fields, these have to be addressed in readObject() and/or readResolve() methods. > Luc > > > > > >> What kind of pain did Serializable cause in MTJ ? > >> > >> BTW, I'm really +1 to Matrix Market files, could you open a Jira issue > >> asking for this enhancement for 2.1 ? > >> > >> > >> > > >> > - I discourage the use of the classes named *Impl. They will get very > >> > confusing when other implementations are added later! Instead, I > recommend > >> > the names ArrayRealVector, Array2DRowRealMatrix (to indicate a 2D array > >> > backed implementation using row ordering). This allows a column-based > or 1D > >> > implementation in the future without names getting very confusing. > These > >> > implementations are hidden from users who just use the MatrixUtils help > >> > >> > >> Phil suggested to change RealMatrixImpl to ArrayRealMatrix (and > >> DenseRealMatrix to BlockRealMatrix). This sounds good to me. > >> > >> > >> > > >> > - -1 for inclusion of methods in the sparse interfaces and shape type > enum. > >> > This needs more thought and discussion before inclusion. I am happy > enough > >> > for marker interfaces to be included (as long as it is documented that > these > >> > interfaces are subject to update in the next release), but strongly > against > >> > including methods in them. > >> > >> > >> +1 to further thoughts and discussion and postponing declaring methods > >> in the interfaces after 2.0. Lets start with empty marker interfaces. > >> > >> > >> Luc > >> > >> > >> > > >> > - could you please describe the hashing algorithm that OpenMap is > using? I > >> > have previously written a GNU Trove based sparse matrix implementation > which > >> > was a map from the primitive "long" to "double". It looks analagous. I > broke > >> > the row/column index into a long for the key using binary operations, > but > >> > the hashcode was tricky as it had to be an integer (not long). A naive > >> > implementation can lead to collisions for typical or symmetric > matrices. It > >> > looks like the current implementation is using an int -> double > backing map! > >> > Wouldn't it make more sense to use a long? > >> > > >> > The following code might be useful to you (don't forget the L marker > and > >> > casts or the compiler will silently cast to an int), there are two > options > >> > for a hashcode calculator... I've not stress tested the second one but > it > >> > might be more appropriate as it gives more weight to lower bits. > Remember > >> > that FF is 256, or a full byte... so a full 64 bit long is 8 bytes or > 16 hex > >> > characters in Java bitwise. > >> > > >> > int hash1(long val) { > >> > int first = Integer.reverse(key2row(val)); > >> > int second = key2col(val); > >> > return first + second; > >> > } > >> > > >> > int hash2(long val) { > >> > int first = key2row(val) & 0x0000FFFF; > >> > int second = key2col(val) << 16; > >> > return first + second; > >> > } > >> > > >> > long key(int row, int column) { > >> > return (((long) row) << 32) + (long) column; > >> > } > >> > > >> > int key2col(long key) { > >> > return (int) (key & 0x00000000FFFFFFFFL); > >> > } > >> > > >> > int key2row(long key) { > >> > return (int) (key >>> 32); > >> > } > >> > > >> > > >> > Bill Barker wrote: > >> >> I have a slight preference for leaving the markers empty until 3.0, > but I > >> >> can remove them as well. But I can wait to see what the community > >> >> consensus > >> >> is before making changes. > >> >> > >> > > >> > >> > >> > >> --------------------------------------------------------------------- > >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > >> For additional commands, e-mail: dev-h...@commons.apache.org > >> > >> > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
