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

add concurrency-friendly map access to fix #8 #9

Merged
merged 3 commits into from Jul 23, 2016

Conversation

piazzamp
Copy link
Collaborator

Created a new type histogram that couples a sync.RWMutex and the existing map. The type itself isn't exported (no one should be instantiating these, right?), but its Get and Set methods are. This is a significant change for consumers that create their own NaiveBayes struct without calling NewNaiveBayes.

@@ -160,6 +161,30 @@ type NaiveBayes struct {
Output io.Writer
}

// histogram allows conncurrency-friendly map access via its
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concurrency*

@cdipaolo
Copy link
Owner

Looks great! Thanks for doing this. I left some comments that should be somehow resolved before this gets merged but they're really small issues.

Thanks! 🐱

@cdipaolo
Copy link
Owner

cdipaolo commented Jul 23, 2016

Also I just remembered: could you add your consistently failing example you sent to the tests (text/bayes_test.go) under the name TestConcurrentPredictionAndLearningShouldNotFail or something similar?

Or rather modify it to make it automatable and then include it in the tests

func newTrainedModel() (*text.NaiveBayes, error) {
    c := make(chan base.TextDatapoint)
    model := text.NewNaiveBayes(c, 9, base.OnlyWords)
    errors := make(chan error)
    go model.OnlineLearn(errors)

    go func() {
        for err, more := <-errors; more; err, more = <-errors {
            if err != nil {
                fmt.Printf("Error passed: %s", err)
            }
        }
    }()
    data, err := trainingData()
    if err != nil {
        return nil, err
    }
    for _, tdp := range data {
        c <- tdp
    }
    close(c)
    return model, nil
}

@cdipaolo cdipaolo merged commit 074aa40 into cdipaolo:master Jul 23, 2016
@cdipaolo
Copy link
Owner

Looks great! I really appreciate all of this. If you feel like helping refactor/improve things/fix another bug any time let me know!!

@piazzamp
Copy link
Collaborator Author

will do - thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants