weecology / LDATS

Latent Dirichlet Allocation coupled with Bayesian Time Series analyses
https://weecology.github.io/LDATS
Other
25 stars 5 forks source link

paper-comparison vignette #124

Closed ha0ye closed 5 years ago

ha0ye commented 5 years ago

Summary of changes:

closes #120

juniperlsimonis commented 5 years ago

are the build breaks here the random number thingy?

ha0ye commented 5 years ago

For the last commit, yes. (I haven’t looked at the logs for all prior commits).

codecov-io commented 5 years ago

Codecov Report

Merging #124 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #124   +/-   ##
=======================================
  Coverage   97.56%   97.56%           
=======================================
  Files          11       11           
  Lines        1191     1191           
=======================================
  Hits         1162     1162           
  Misses         29       29

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 bf918db...fca81b5. Read the comment docs.

juniperlsimonis commented 5 years ago

finally got back to this (sorry!) and with rebasing this branch with the updates you had in the other pr, @ha0ye it looks like that takes care of the test issue (woo hoo!), which was my concern here. @diazrenata do you have any editing feedback or can this be merged in?

juniperlsimonis commented 5 years ago

do note that we'll need to update some of the code and model objects with the jump to v0.2.0 (ref: #131 ), so i'm not sure which we want to do first?

ha0ye commented 5 years ago

do note that we'll need to update some of the code and model objects with the jump to v0.2.0 (ref: #131 ), so i'm not sure which we want to do first?

I think this PR is independent enough (it only changes 2 files) that it can be rebased on top of 0.2.0; and the vignette should be restructured to make the changes to the code and model straightforward in terms of a patch or a few additional commits.

juniperlsimonis commented 5 years ago

that makes sense. the thing we'll definitely need to keep an eye out for though is updated model output structure that updated code is pointed to (the timename element in the TS output in particular), as we'll need to update model objects wherever they live. i've updated the model objects in the v0.2.0 branch, so the ones you are calling up remotely here can get updated from those ones, yeah?

do note that we'll need to update some of the code and model objects with the jump to v0.2.0 (ref: #131 ), so i'm not sure which we want to do first?

I think this PR is independent enough (it only changes 2 files) that it can be rebased on top of 0.2.0; and the vignette should be restructured to make the changes to the code and model straightforward in terms of a patch or a few additional commits.