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

stable release #24

Open
daydreamt opened this issue May 11, 2013 · 12 comments
Open

stable release #24

daydreamt opened this issue May 11, 2013 · 12 comments

Comments

@daydreamt
Copy link
Contributor

Hey there,

Incanter is depending on clatrix for both QR decomposition and the determinant function,
and since there is no recent non-SNAPSHOT version of clatrix yet, the latest Incanter version has no QR decomposition or determinant functions.

incanter/incanter#122

What would it take to make a stable release? More SVD tests?

@ejackson
Copy link
Collaborator

Does Incanter build and test with the current snapshot ?

@ejackson
Copy link
Collaborator

The parts of Clatrix upon which Incanter depends haven't changed much in a while. The only movement is in tracking core.matrix, so I'm pretty happy with stability. Others ?

@daydreamt
Copy link
Contributor Author

I'm afraid it does not.
The only change I could see was that the identity matrix function was renamed from id to eye.
However, the tests fail.
Here is travis running them:
https://travis-ci.org/daydreamt/incanter

I will dig deeper later to ensure I haven't done anything stupid.

@daydreamt
Copy link
Contributor Author

So, I went through the failed tests and the error.
The tests failed because of API changes, mostly.

(def A [ [1 2 3], [4 5 6], [7,8,9] ,[10,11,12]]

(conj A [13 14 15])
;=> now returns nil, didn't use to

(conj A [[13,14,15]])
;=> A 5x3 matrix
;=>  -------------
;=> 1.00e+00  2.00e+00  3.00e+00 
;=> 4.00e+00  5.00e+00  6.00e+00 
;=> 7.00e+00  8.00e+00  9.00e+00 
;=> 1.00e+01  1.10e+01  1.20e+01 
;=> 1.00e+00  2.00e+00  3.00e+00 
;something like this was returned before, I think

;The next error occurs due to some change in the matrix function. I think.
;;(def M (predict (simple-regression [2 4] [1 3]) 2))
;which returns
(def M (matrix [3]))
;=>  A 1x1 matrix
;=> -------------
;=> 3.00e+00 

(= 3.0 M)
;=> false, but used to be true

;The last one I have to think about. It has to do with vector multiplication.

The serious error is due to the first change, I think.
It is not expected behaviour, right?

@alexott
Copy link

alexott commented May 18, 2013

If new Clatrix will be release, I'll incorporate it into Incanter immediately...

@mikera
Copy link
Collaborator

mikera commented May 27, 2013

My hacking to make add core.matrix support to Incanter will probably also depend on a new Clatrix release.

I can push out a release to net.mikera/clatrix if useful as a temporary measure?

@alexott
Copy link

alexott commented May 27, 2013

Yes, I think that this could work. Although I've discussed this with Edmund & Joseph - they're also ready to push new release when I do changes into Incanter that allow to work with new version

@ejackson
Copy link
Collaborator

OK, sorry for the absence. Back now.

  1. It seems that cons is incorrectly implemented. Its returning a concrete type where it should be returning a sequence. We need to change that, but its going to cause breakage.
  2. Conj is unimplemented. I'm not sure how to do this, can you give me pointers please ? As I understand it is not in any of the core interfaces or existing protocols. Should we create our own protocol and extend it Matrix and Vector ?
  3. (not (= 3 (matrix [3]))) is the correct behaviour, I think. Agree ?

@mikera
Copy link
Collaborator

mikera commented May 27, 2013

The whole "treat a matrix like a sequence of sub-matrices" concept appears to be a bit problematic from what I've observed in my experiments to get Incanter working with core.matrix. There's a lot of inconsistency lurking around which makes it hard to define correct behaviour.

If we're going to cause breakage anyway, I think it would be worth revisiting the design in this area.

@ejackson
Copy link
Collaborator

I don't like it myself, how much is it used in Incanter ?

@mikera
Copy link
Collaborator

mikera commented May 28, 2013

Hard to say: you don't need it (there are always other ways of achieving the same thing) but it's quite widely used in the docstrings and tests.

I suggest:

  1. Being more explicit when you actually want a sequence of rows or columns, e.g. telling people to use (first (columns A)) or (map something (rows A))
  2. Mimicking regular Clojure vectors / core.matrix behaviour, e.g. a row is considered to be a 1D Vector rather than a 1xN 2D Matrix. This also matches people's intuitive expectation about arrays in other languages. I believe this is the best way to ensure consistent behaviour

(1) can be done quite easily, just by deprecating the old usage and changing docstrings. (2) is relatively easy to do as well, but will almost certainly cause breakage, so we should probably save it for a major (2.0 ?) release of Incanter and corresponding (3.0? 4.0?) Clatrix release

@alexott
Copy link

alexott commented May 31, 2013

I've fixed the problem with bind-rows & fresh clatrix. The only remaining issue is: conj of matrix with vector that is described above.

If we can fix this issue, then I can cut the 1.5.0 release of Incanter & after that we can concentrate on 2.0 based on core.matrix.

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

No branches or pull requests

4 participants