-
Notifications
You must be signed in to change notification settings - Fork 51
Update jax requirement from !=0.7.1,<0.10.0,>=0.6.2 to >=0.6.2,!=0.7.1,<0.11.0 in the jax group across 1 directory #2235
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
base: master
Are you sure you want to change the base?
Changes from all commits
a71b715
9adb60f
05b3097
a285c98
69acac3
4c32861
50477a2
8c4a708
e76b870
89abded
9ff1025
bc2ca4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -724,6 +724,7 @@ def filter(z1, z2): | |
| return z1[mask], z2[mask] | ||
|
|
||
| @pytest.mark.unit | ||
| @pytest.mark.xfail(reason="will be fixed by #2199") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the same problem as the above link, except this PR's approach is to break the code by releasing with a package that does not pass correctness tests.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed this in the dev meeting, and the decision was to add this flag temporarily. Small numerical differences in different versions of dependencies can already be causing problems that we haven't seen. For example, this workflow fails due to jax-finufft=1.2.0, currently this workflow is the only one that uses that version because There won't be a release very soon. In the meantime, we can merge the proper fix #2199. I can approve that PR and we can merge it soon, but it would be a lot easier if you spent a couple of minutes to cherry-pick a couple of commits to point it to master. Otherwise, it will wait on a chain of PRs. I can say the same for couple other PRs too. You can keep the original PRs open, I am pretty sure if you merge the exact same changes to master, there won't be merge conflicts.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worked on DESC for 4 years. I will block PRs that 1) show failure in the correctness of the code, and 2) remove required testing infrastructure as means to silence such failures. The other things you mentioned are non-sequitur. Nothing is holding up the PRs you mentioned. My existing PRs that allow the code to work in a broader range of configurations have been ready for merge with no issues.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| def test_z1_first(self): | ||
| """Case where straight line through first two intersects is in epigraph.""" | ||
| start = np.pi / 3 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider explaining this issue in
tr_methoddocs for QR. If there is a chance that the jacobian is singular, then SVD is the more reliable option. If the problem is well-posed otherwise, having a wideboundscan cause this (some of the columns are completely 0).