Skip to content
This repository has been archived by the owner on Jun 14, 2023. It is now read-only.

Testing BW and more #6

Merged
merged 8 commits into from
Aug 13, 2019
Merged

Testing BW and more #6

merged 8 commits into from
Aug 13, 2019

Conversation

nantonel
Copy link
Contributor

@nantonel nantonel commented Jun 7, 2019

Re-written many functions using in-place versions, changed names of parameters, added new tests including to Baum Welch. Should take into account most of the issues of #1. Benchmarks not updated yet.

@nantonel nantonel changed the title Substantial refactoring Testing BW and more Jun 7, 2019
@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #6 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
- Coverage   97.53%   97.46%   -0.07%     
==========================================
  Files           6        5       -1     
  Lines         162      237      +75     
==========================================
+ Hits          158      231      +73     
- Misses          4        6       +2
Impacted Files Coverage Δ
src/HMMBase.jl 100% <ø> (ø) ⬆️
src/messages.jl 100% <100%> (ø) ⬆️
src/hmm.jl 100% <100%> (+10.52%) ⬆️
src/baum_welch.jl 100% <100%> (ø)
src/utilities.jl 0% <0%> (-100%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4c2ec2...6266a95. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #6 into master will increase coverage by 0.82%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   96.63%   97.46%   +0.82%     
==========================================
  Files           6        5       -1     
  Lines         119      237     +118     
==========================================
+ Hits          115      231     +116     
- Misses          4        6       +2
Impacted Files Coverage Δ
src/HMMBase.jl 100% <ø> (ø) ⬆️
src/messages.jl 100% <100%> (ø) ⬆️
src/hmm.jl 100% <100%> (+11.11%) ⬆️
src/baum_welch.jl 100% <100%> (ø)
src/utilities.jl 0% <0%> (-100%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9b11ec...724d418. Read the comment docs.

@maxmouchet
Copy link
Owner

Thanks for the PR! Give me a few days to review the changes :-)
Changing the fields and functions names is a big changes but yours are indeed more consistent. Since we're still at an early stage this may be doable :-)

@nantonel
Copy link
Contributor Author

yes, that's what I had in my mind when I've made these mods! 😃

@maxmouchet
Copy link
Owner

maxmouchet commented Jun 11, 2019

The tests fails on my installation with Distributions v0.16.4:

suffstats is not implemented for (DiscreteNonParametric{Int64,Float64,Base.OneTo{Int64},Array{Float64,1}}, Array{Int64,1}, Array{Float64,1})
suffstats is not implemented for (Gamma{Float64}, Array{Float64,1}, Array{Float64,1})

However they pass with the latest version v0.20.0. Can you add the following in Project.toml:

[compat]
Distributions = ">= 0.20.0"

Thanks!

@nantonel
Copy link
Contributor Author

should be fixed now! 😉

@codecov-io
Copy link

codecov-io commented Jul 5, 2019

Codecov Report

Merging #6 into master will increase coverage by 0.82%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   96.63%   97.46%   +0.82%     
==========================================
  Files           6        5       -1     
  Lines         119      237     +118     
==========================================
+ Hits          115      231     +116     
- Misses          4        6       +2
Impacted Files Coverage Δ
src/HMMBase.jl 100% <ø> (ø) ⬆️
src/messages.jl 100% <100%> (ø) ⬆️
src/hmm.jl 100% <100%> (+11.11%) ⬆️
src/baum_welch.jl 100% <100%> (ø)
src/utilities.jl 0% <0%> (-100%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9b11ec...724d418. Read the comment docs.

@maxmouchet
Copy link
Owner

Hi, I'm a bit busy for the moment but I haven't forgotten you :-)

@maxmouchet maxmouchet changed the base branch from master to refactor_v1 August 13, 2019 15:40
@maxmouchet maxmouchet merged commit 724d418 into maxmouchet:refactor_v1 Aug 13, 2019
@maxmouchet
Copy link
Owner

I've rebased your changes in the refactor_v1 branch where I'll prepare the release of the v1.0.
I want to make a few cosmetic changes to the code before merging to master.

@maxmouchet
Copy link
Owner

Hi, I finally released the new version of the library, including the renaming of the HMM fields.

I did not merged all of your changes as-is, as I wanted to preserve some of the original design decisions (AbstractHMM types parameters, for example), and the PR was quite big with intertwined commits.

I mentioned you where I thought it was appropriate (release notes, messages.jl and viterbi.jl files).

Thanks for your improvements, your implementations were much cleaner and faster :-)

@nantonel
Copy link
Contributor Author

That's great! Thank you! 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants