Skip to content

Reenable tests that were mostly good already #4830

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

Merged
merged 6 commits into from
Jul 2, 2021

Conversation

michaelosthege
Copy link
Member

This PR includes more test files in the CI actions.
Only three remain excluded:

I also split up the test jobs based on their runtime. This was before, labeled by the name of the first test in each job.
grafik

The test_distributions.py is by far the largest single one. We should definitely split it into two or three files and run them separately!!

@ricardoV94
Copy link
Member

ricardoV94 commented Jul 1, 2021

The test_distributions.py will be significantly faster after #4736

I'll try to split the speedup part into a separate PR

@@ -23,7 +23,7 @@ def test_profile_model(self):
assert self.model.profile(self.model.logpt).fct_call_time > 0

def test_profile_variable(self):
assert self.model.profile(self.model.value_vars[0].logpt).fct_call_time > 0
assert self.model.profile(self.model.value_vars[0]).fct_call_time > 0
Copy link
Member

@ricardoV94 ricardoV94 Jul 2, 2021

Choose a reason for hiding this comment

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

This one does not seem equivalent...

Suggested change
assert self.model.profile(self.model.value_vars[0]).fct_call_time > 0
rv = self.model.basic_RVs[0]
self.model.profile(pm.logpt(rv, self.model.rvs_to_values[rv])).fct_call_time

Copy link
Member Author

Choose a reason for hiding this comment

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

It returns the TensorVariable which is then profiled, no?
Can you suggest a change? As you can see I'm still confused by these logpt things..

Copy link
Member

Choose a reason for hiding this comment

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

I added a suggestion in my first comment, but it has to be tested locally, as I might have screwed up the names while copy pasting from the REPL

A few XFAILs were added because DensityDist is not yet refactored and a shape issues causes the CompoundStep to fail.
This way they don't XPASS sometimes.
Also it doesn't waste compute resources on tests whoose outcome has no relevance.
@ricardoV94
Copy link
Member

ricardoV94 commented Jul 2, 2021

This failing test seems pretty close to the expected result https://github.com/pymc-devs/pymc3/pull/4830/checks?check_run_id=2971381082#step:7:199

But if you keep finding issues it might be related to #4771

@michaelosthege michaelosthege merged commit 13487d0 into pymc-devs:main Jul 2, 2021
@michaelosthege michaelosthege deleted the reenable-tests branch July 2, 2021 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants