fcanas / Bayes

Naive Bayes Classifier in Swift for Mac and iOS
MIT License
32 stars 7 forks source link

change from struct to class, any issues? #16

Closed Mamonaku closed 6 years ago

Mamonaku commented 6 years ago

Hi Fabien, thanks for the great code. For a personal project, I need to use BayesianClassifier through a computed var, with get access only. Unfortunately, because it's a struct, and it's mutated when calling .observe(), the compiler won't let me do it....

Changing struct to class seems to do the job. I'm however unsure about the consequence.

Would you have any recommendations? thanks! -M

fcanas commented 6 years ago

Hi Mamonaku.

I'm not sure of your background in Swift, so let me know if something doesn't make sense, or forgive me if I'm going over things you're already familiar with.

You could reconsider the get-only requirement on the classifier var.

Another option is to make a class with a var BayesianClassifier that exposes the interface you need and forwards calls to its private classifier. A var for this new class could be read-only without any problem. Or you could do this with

You can change BayesianClassifier to a class without much repercussion internally. I don't want that merged back into the project at this time since it can significantly change how it's used. In particular, I used structs for these to be able to keep states before and after an event independently.

Mamonaku commented 6 years ago

Hi Fabian, No problem, all advices are welcome! Yes, struct are preferable, I wish I had thought about your option before, but at this point, it will require a little bit of refactorisation, which I'd rather avoid. Or in other words, if it's not broken, don't fix it :-)

I've tested everything with your tests, all passed, but I wanted to check for edge cases with the expert before.

btw, do you know a good implementation of Multimap in swift? There are plenty on GitHub, just wondering if you tried any?

thanks for your help, -M

fcanas commented 6 years ago

I have not evaluated any swift multimap implementations. I’d be inclined to roll my own with a dictionary of sets.

-F