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

Test on latest Julia version (~1.8) #188

Merged
merged 2 commits into from
Sep 16, 2022
Merged

Test on latest Julia version (~1.8) #188

merged 2 commits into from
Sep 16, 2022

Conversation

ilopezgp
Copy link
Contributor

PULL REQUEST

Purpose and Content

Update package to be compatible with Julia v1.8. Compatibility does not seem to be automatic, since our tests are not passing.

@odunbar
Copy link
Collaborator

odunbar commented Aug 26, 2022

Note - the test Test / Julia 1 - ubuntu-latest - x64 failed previously with a SingularException.

On re-running tests (with no change) this now passes, along with macOS-latest,

but now windows-latest fails with the same SingularException.

So there may be some instability in the linear solvers from LinearAlgebra, as I don't think the problem is really singular. I believe someone has recommended linearsolvers.jl in the past, maybe this is a way forward if these tests still give us issues

@ilopezgp
Copy link
Contributor Author

Note - the test Test / Julia 1 - ubuntu-latest - x64 failed previously with a SingularException.

On re-running tests (with no change) this now passes, along with macOS-latest,

but now windows-latest fails with the same SingularException.

So there may be some instability in the linear solvers from LinearAlgebra, as I don't think the problem is really singular. I believe someone has recommended linearsolvers.jl in the past, maybe this is a way forward if these tests still give us issues

Thoughts on this issue, @charleskawczynski ? We are getting SingularExceptions in linear systems that are not singular by Julia <1.8 standards.

@charleskawczynski
Copy link
Member

Thoughts on this issue, @charleskawczynski ? We are getting SingularExceptions in linear systems that are not singular by Julia <1.8 standards.

I'm not sure there's enough information to debug. What's the stack trace? Is this an error in TC or in CEDMF? We haven't started testing TC with 1.8 yet, so it might make sense to do that first before upgrading this repo.

I think we're seeing similar periodic windows errors in TC-- which, IIRC, is some sort of memory mismanagement, and I'm not exactly sure what's going on there. I've not been able to reproduce locally

@ilopezgp
Copy link
Contributor Author

Thoughts on this issue, @charleskawczynski ? We are getting SingularExceptions in linear systems that are not singular by Julia <1.8 standards.

I'm not sure there's enough information to debug. What's the stack trace? Is this an error in TC or in CEDMF? We haven't started testing TC with 1.8 yet, so it might make sense to do that first before upgrading this repo.

I think we're seeing similar periodic windows errors in TC-- which, IIRC, is some sort of memory mismanagement, and I'm not exactly sure what's going on there. I've not been able to reproduce locally

This error is localized to EKP.jl, it comes up in a unit test independent of TC or CEDMF. Basically, the error comes from the operation:

result = (cov_mat) \ (vector)

which throws a SingularException (associated with cov_mat) that we did not see in Julia 1.7.3.

@charleskawczynski
Copy link
Member

Can you make a reproducer of the values used?

@ilopezgp
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Sep 11, 2022
@bors
Copy link
Contributor

bors bot commented Sep 11, 2022

try

Build failed:

@odunbar
Copy link
Collaborator

odunbar commented Sep 14, 2022

update:

Looking at the dumped matrices we find that \ works on 1.7.2 and not 1.8.0 because the matrices are near ill-conditioned. Operations cond(A), and det(A) give different answers too.

One option

After suggestion of @charleskawczynski , replacing

tmp = FT.((cov_gg + obs_noise_cov) \ (y - g))

with

LHS = Matrix{BigFloat}(cov_gg + obs_noise_cov)
RHS = Matrix{BigFloat}(y - g)
tmp = FT.(LHS \ RHS )

leads to tests passing in 1.8.0

Of course this is not ideal, as it is likely a far less optimized solve method when using arbitrary precision.

@ilopezgp
Copy link
Contributor Author

We could implement that in a try catch. As you said, I think that will be less efficient in general settings.

@ilopezgp
Copy link
Contributor Author

We could implement that in a try catch. As you said, I think that will be less efficient in general settings.

Done and solved! I will merge this.

@ilopezgp
Copy link
Contributor Author

bors r+

@ilopezgp ilopezgp marked this pull request as ready for review September 16, 2022 04:16
@bors
Copy link
Contributor

bors bot commented Sep 16, 2022

Build succeeded:

@bors bors bot merged commit b412775 into main Sep 16, 2022
@bors bors bot deleted the try_1.8 branch September 16, 2022 04:28
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