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

chore(ci): disable continue-on-error for all test jobs in CI workflow #1968

Merged
merged 7 commits into from
Feb 3, 2025

Conversation

Qazalbash
Copy link
Contributor

continue-on-error was temporarily introduced in #1959 and accidentally got merged into the default branch. This PR reverts the commit (295136f) introducing the temporary changes.

@Qazalbash
Copy link
Contributor Author

@fehiepsi Some more test cases have failed!

Can you check test/contrib/einstein/test_stein_loss.py::test_stein_particle_loss? It has a relatively large error.

@fehiepsi
Copy link
Member

fehiepsi commented Feb 1, 2025

@OlaRonning could you take a look? i think the inspected zs is different now.

@OlaRonning
Copy link
Member

I think you're right @fehiepsi. I'll work it over in detail in the morning.

Updated latents in stein loss test case
@@ -80,7 +80,7 @@ def stein_loss_fn(chosen_particle, obs, particles, assign):
xs = jnp.array([-1, 0.5, 3.0])
num_particles = xs.shape[0]
particles = {"x": xs}
zs = jnp.array([-0.1241799, -0.65357316, -0.96147573]) # from inspect
zs = jnp.array([-3.3022664, -1.06049, 0.64527285]) # from inspect
Copy link
Member

Choose a reason for hiding this comment

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

could we just replicate the logic to generate zs here? @OlaRonning

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that should be possible. I'll have a look at it now.

changed zs to be computed instead of hardcoded
@OlaRonning
Copy link
Member

The stein test is passing but there is a problem with stochastic support.

@Qazalbash
Copy link
Contributor Author

I fixed the very same problem for py3.10 in 78c51a7, by changing the random seed, because the error was too high for the previous seed on which it was passing on py3.9. Now it is passing for py3.10 and not for py3.9. I presume we will encounter such cases in the future too! We have to find something more robust that will work on both Python versions.

@OlaRonning
Copy link
Member

Looking at the failing assert in CI it looks like the dimensions are swapped. Could be spurious or could be that the inference in the test case is nonidentifiable. Note: I haven't check the test or method in detail.

@fehiepsi
Copy link
Member

fehiepsi commented Feb 3, 2025

Yeah, we just need to assert whether the actual is close to expected or expected[::-1] (using np.is_close)

@OlaRonning
Copy link
Member

OlaRonning commented Feb 3, 2025

This seems like a simpler solution than making the toy problem identifiable 👍 I'll add it.

Allow for both solutions in test/contrib/stochastic_support/test_dcc.py:
fixed tolerance
Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

Thanks @Qazalbash and @OlaRonning!

@fehiepsi fehiepsi merged commit d6ba568 into pyro-ppl:master Feb 3, 2025
10 checks passed
@Qazalbash Qazalbash deleted the ci-remove-continue-on-error branch February 3, 2025 16:35
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.

3 participants