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

AbstractReTerm #395

Merged
merged 5 commits into from Oct 4, 2020
Merged

AbstractReTerm #395

merged 5 commits into from Oct 4, 2020

Conversation

palday
Copy link
Member

@palday palday commented Oct 1, 2020

Closes #293. Not sure if this is a good or bad idea. Thoughts, @kleinschmidt ?

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #395   +/-   ##
=======================================
  Coverage   95.32%   95.32%           
=======================================
  Files          23       23           
  Lines        1605     1605           
=======================================
  Hits         1530     1530           
  Misses         75       75           
Impacted Files Coverage Δ
src/randomeffectsterm.jl 94.11% <ø> (ø)

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 5d3988d...ef67e39. Read the comment docs.

@dmbates
Copy link
Collaborator

dmbates commented Oct 1, 2020

Should FullDummy also be an AbstractReTerm?

@palday
Copy link
Member Author

palday commented Oct 1, 2020

@dmbates Good question. I'm hoping @kleinschmidt has a strong feeling on that, but I'll give it a think. The only downside I see is that StatsModels also has fulldummy for any categorical predictor and fulldummy is defined at the level of a single categorical experimental variable, while zerocorr is approximately at the level of a grouping variable.

Copy link
Member

@kleinschmidt kleinschmidt left a comment

Choose a reason for hiding this comment

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

If this is just for dispatch then it seems harmless to me. There's no struct to represent full dummy coding (it just creates a new CategoricalTerm using FullDummyCoding) so that's not an issue. RandomEffectsTerm doesn't seem to be used elsewhere in the code (e.g. to restrict what can be stored as the reterms in the model struct) so no objections there either.

Copy link
Collaborator

@dmbates dmbates left a comment

Choose a reason for hiding this comment

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

Other than the URL change, it looks good.

There is a script in the docs/ directory to create the footnotes automatically.

NEWS.md Outdated Show resolved Hide resolved
Co-authored-by: Douglas Bates <dmbates@gmail.com>
@palday palday merged commit 52874ee into master Oct 4, 2020
@palday palday deleted the pa/zerocorrabstractre branch October 4, 2020 18:44
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.

Question on ZeroCorr term / AbstractRandomEffectsTerm
3 participants