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.

Sign up for GitHub

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

Jump to bottom

Make DiscreteResults Unchanging #4322

Merged
merged 1 commit into from
May 7, 2019
Merged

Make DiscreteResults Unchanging #4322

merged 1 commit into from
May 7, 2019

Conversation

jbrockmendel
Copy link
Contributor

It is really tough to figure out what is tested where, what has externally-confirmed tests vs internal-consistency tests vs smoketests. This peels back one entirely unnecessarily layer of that.

@coveralls
Copy link

coveralls commented Mar 7, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-0.006%) to 84.61% when pulling 0512146 on jbrockmendel:btests into 116cdd8 on statsmodels:master.

@codecov-io
Copy link

codecov-io commented Mar 7, 2018 •
edited by codecov bot
Loading

Codecov Report

Merging #4322 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4322      +/-   ##
==========================================
- Coverage    82.1%   82.09%   -0.01%     
==========================================
  Files         587      587              
  Lines       93446    93409      -37     
  Branches    10372    10372              
==========================================
- Hits        76722    76685      -37     
  Misses      14327    14327              
  Partials     2397     2397
Impacted Files Coverage Δ
statsmodels/discrete/tests/test_discrete.py 99.32% <100%> (-0.02%) ⬇️
statsmodels/discrete/tests/test_count_model.py 100% <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 116cdd8...0512146. Read the comment docs.

@josef-pkt josef-pkt reopened this Apr 3, 2018
@josef-pkt
Copy link
Member

josef-pkt commented Apr 3, 2018 •
edited
Loading

(found this open in a browser tab that I looked at)

I think main idea is good.
some renamings to make it closer to the usual Stata results files
Namespace -> Holder
obj -> res

and I think it's better without attributes, i.e. use method call which will load the relevant data in a lazy way, instead of attaching all results to the classes upon import.
(actually, when we use the Holder class directly, then we don't load the data lazily either.)

@josef-pkt josef-pkt added this to the 0.10 milestone Apr 3, 2018
@josef-pkt
Copy link
Member

not urgent, maybe eventually

@josef-pkt josef-pkt modified the milestones: 0.10, 0.11 Sep 16, 2018
bashtage
bashtage reviewed May 6, 2019
View reviewed changes
Copy link
Member

@bashtage bashtage left a comment

Choose a reason for hiding this comment

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

Not sure about this, even though milestoned by @josef-pkt

statsmodels/discrete/tests/test_count_model.py Show resolved Hide resolved
@bashtage bashtage self-assigned this May 6, 2019
@bashtage
Copy link
Member

bashtage commented May 6, 2019

Needs rebase, pls.

@bashtage bashtage requested review from bashtage and removed request for bashtage May 6, 2019 20:10
bashtage
bashtage reviewed May 6, 2019
View reviewed changes
statsmodels/discrete/tests/test_count_model.py
@@ -98,8 +97,7 @@ def setup_class(cls):
cls.res1._results._attach_nullmodel = True
cls.init_keys = ['exog_infl', 'exposure', 'inflation', 'offset']
cls.init_kwds = {'inflation': 'probit'}
res2 = RandHIE()
res2.zero_inflated_poisson_probit()
res2 = RandHIE.zero_inflated_poisson_probit
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

@bashtage
Copy link
Member

bashtage commented May 7, 2019

lint failure.

@jbrockmendel
Copy link
Contributor Author

azure fail is get_rdataset URLError

@bashtage bashtage merged commit ac400cf into statsmodels:master May 7, 2019
@jbrockmendel jbrockmendel deleted the btests branch May 7, 2019 21:57
@bashtage bashtage mentioned this pull request Jun 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bashtage bashtage bashtage approved these changes

Assignees

@bashtage bashtage

Labels
comp-discrete type-refactor type-test
Projects
None yet
Milestone
0.11
Development

Successfully merging this pull request may close these issues.
5 participants
@jbrockmendel @coveralls @codecov-io @josef-pkt @bashtage