convert to Swift3 syntax

[thomaskilian]

Hi Scott, As requested, here is the PR for the Swift3 part.

[stsievert]

All the tests pass, except the IO tests (which can be run with run_io_tests: true). Could you fix this?

[thomaskilian]

This is probably because of a hard coded path (IIRC; which I had to fit to my machine). I'll have a look into that.

[stsievert]

I ran into some issues with the Swift 3 conversion... it looked like certain things weren't supported or changed their interface

[thomaskilian]

Can you share the issues somehow? Hard to imagine for me what's going on over there :-)

[stsievert]

Before:

func read_csv(_ filename:String, header_present:Bool=true, _ rowWithoutMissingValues: Int=1) -> CSVFile{
    var x: String?
    do {
        x = try String(contentsOfFile: filename, encoding: String.Encoding.utf8)
    } catch _ {
        x = nil
    }
    // ...
// prints 
// Error writing to CSV: filename probably wasn't recognized.
// fatal error: unexpectedly found nil while unwrapping an Optional value

After I fixed this:

var y:CSVFile = try read_csv("/Users/scott/Desktop/test_2016.csv", header_present:true)
    var x: String?
    x = try String(contentsOfFile: filename, encoding: String.Encoding.utf8)
    // ...

[thomaskilian]

Yes. And of course this path won't work for me (or anyone else than you) :-) There is a NS-method to get the desktop path. I'll have a look...

[thomaskilian]

Try this

let documentsPath = NSSearchPathForDirectoriesInDomains(.desktopDirectory, .userDomainMask, true)[0]
print(documentsPath)

[thomaskilian]

I have added this and it should now run for anyone.

[stsievert]

Do the IO tests pass? read_csv, write_binary, etc?

[thomaskilian]

Yes, they do. I added the above path computation. Sorry that I speed up so much. I'll try to pass you slices of my work on this branch. I got hold of the functional/OO part and am currently working on that.

[stsievert]

I'm not sure I'm a fan of the function/OO framework -- maybe open a work in progress PR where we can discuss it while it's being worked on?

[thomaskilian]

Actually I'm no fan of the (pure) functional approach. Actually I don't need it for myself since it's simply no good programming style. However, there's a simple way to provide functional access for those who like it for some reason. So for people wanting a functional approach there's a slightly different framework being generated. From a framework development perspective there is not much overhead. E.g. sum will only be available as vector.sum() in the OO framework but sum(vector) will additionally be available in the functional framework simply by providing a wrapper function

public func sum(_ x: Vector) -> Double{ return x.sum() }

[stsievert]

On my machine on your branch, the IO tests don't pass. When I change run_io_tests:false to run_io_tests:true in main.swift, in read_csv (on line 100 in io.swift) I get an error about

fatal error: unexpectedly found nil while unwrapping an Optional value
2016-10-19 16:58:58.524406 swix[58050:12382301] fatal error: unexpectedly found nil while unwrapping an Optional value

[thomaskilian]

I'll have a look tomorrow (sleeping time now...). I'm not really happy with the tests anyway. There should be a full test each time. So I did not do the test with that change.

[thomaskilian]

You reference ../../python_testing/csvs/image.csv. How can you assume that this path exists at all? There is no precaution made. No wonder this test fails. Also: why should one assume that tests have to be run with run_io_tests:true. Doing regression tests must take all test paths and not only after modifying code. Shall I raise an issue for that? Would you fix it?

[stsievert]

This is why I defaulted run_io_tests to be false: I didn't want to make that assumption. I had a python script that could make sure values could be passed between Python and Swift.

I would wait before modifying the tests -- there are still issues to be resolved in the IO tests and I want to reformat for the Swift package manager (issue #23).

[thomaskilian]

Have you thought about using XCTest? In my current build I have compressed all those tests to occur i na single app (just as a quick fix). But to make sure that all these math operations perform correctly, there needs to be some sort of full blown regression test. One that does not make assumptions about external stuff like python directories. It would probably be a good idea, to decouple the python-dependant parts from the rest. That is also related to the hard-coded python-paths which occur in the code. E.g. I don't have anaconda but active state python on my machine. Others probably have brew python libs.

[stsievert]

No, I haven't heard about XCTest. I wrote just after Swift had been released and the environment was still developing.

The IO tests still need to be converted to Swift 3. That should come first and this PR should only cover that.

[thomaskilian]

This part is converted correctly to Swift3. However, your basic code does not work here. I changed the paths to work everywhere as you may note.

write_csv(x1, filename: "/tmp/image.csv")
let y1:matrix = read_csv("/tmp/image.csv").data

But since

func read_csv(_ filename:String, header_present:Bool=true, _ rowWithoutMissingValues: Int=1) -> CSVFile{

the read will try to read a header, because header_present:Bool=true. This will swallow the first row of the matrix. I can't verify the original since XCode does not accept the old Swift. But that would need a fix in your code to.

However, if you integrate this PR, I'll make a fix for this.

[thomaskilian]

I have fixed the tests and now they run each time

[stsievert]

Good job! 👍

[thomaskilian]

I'll setup a new branch for the functional/OO approach, so that can exist along the main branch until maybe you are convinced to merge it to master :-) However, before that I will clean up the testing. This simply because I ran into an issue with my f/OO transition (which else works fine).

[thomaskilian]

Working on the OO/F I have a question:

func max(_ x: matrix, axis:Int = -1)->Double{
  return x.max()
}

does not use the axis parameter. Is that correct?