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

CI with Julia v1.11 #3836

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

CI with Julia v1.11 #3836

wants to merge 21 commits into from

Conversation

navidcy
Copy link
Collaborator

@navidcy navidcy commented Oct 8, 2024

This PR switches the CI to use Julia v1.11.
It also adds a Manifest with v1.11 ending so that there is still compatibility with previous versions.

Note the the distributed CI still does not have Julia v1.11 (right @Sbozzolo?) so there Julia v1.10 is used. This is possible because there are two Manifests.

@navidcy navidcy added testing 🧪 Tests get priority in case of emergency evacuation package 📦 Quite meta labels Oct 8, 2024
@Sbozzolo
Copy link
Member

Sbozzolo commented Oct 8, 2024

Note the the distributed CI still does not have Julia v1.11 (right @Sbozzolo?) so there Julia v1.10 is used. This is possible because there are two Manifests.

I installed julia 1.11 on the Caltech cluster, but we haven't made a module yet (but it's coming today or so)

@glwagner
Copy link
Member

glwagner commented Oct 9, 2024

This is possible because there are two Manifests.

There's only one manifest?

@ali-ramadhan
Copy link
Member

This is possible because there are two Manifests.

There's only one manifest?

I assume he's referring to this new feature of Julia 1.11: https://julialang.org/blog/2024/10/julia-1.11-highlights/#manifest_versioning

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 9, 2024

There's only one manifest?

The PR adds another Manifest specifically for v1.11 and keeps the older Manifest that works for v1.10. Or we can just have one Manifest (the one for v1.11) and drop the one for v1.10

@glwagner
Copy link
Member

glwagner commented Oct 9, 2024

keeps the older Manifest that works for v1.10

The Manifest was deleted in #3783

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 9, 2024

oh great! I missed that!
I wanted to delete it long now but I got resistance doing that before... Great! So I'll delete the new Manifest then as well!

@glwagner
Copy link
Member

glwagner commented Oct 9, 2024

oh great! I missed that! I wanted to delete it long now but I got resistance doing that before... Great! So I'll delete the new Manifest then as well!

Deleting it seemed to help increase the likelihood that CI passed. Although, it did not fully solve the problem (and note a few other changes were also made on #3783).

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 11, 2024

Noting that internal_tide.jl gives NaN with Julia v1.11 while all is OK with Julia v1.10; something with immersed boundaries....? I'm looking into it.

@simone-silvestri
Copy link
Collaborator

I think it's a plotting issue. We are filling up the immersed boundaries with NaN and, apparently, we cannot plot NaNs anymore? The error says:

ERROR: LoadError: On worker 2:
  | Looking up a non-finite or NaN value in a colormap is undefined.

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 11, 2024

I ran the script and the actual simulation NaN-ed.

@glwagner
Copy link
Member

I ran the script and the actual simulation NaN-ed.

That means Oceanangians isn't compatible with julia 1.11.

Do any other tests catch the issue? We can use this opportunity to add more tests.

@navidcy
Copy link
Collaborator Author

navidcy commented Oct 11, 2024

I’m trying to make an mwe

@ali-ramadhan
Copy link
Member

Weird that this one example is behaving quite differently.

In Julia 1.10.6, the example produces flows that generally remain at a max CFL of ~0.13 or lower.

In Julia 1.11.1, the flow reaches a max CFL of ~0.2 after ~60 iterations and blows up within ~100 iterations.

Took a quick look but couldn't find anything obviously different.

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

@glwagner
Copy link
Member

glwagner commented Oct 30, 2024

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

There are regression tests, but not sure if they are with immersed boundary or not.

@simone-silvestri
Copy link
Collaborator

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

There are some tests but quite fragmented, not very exhaustive and that run only for 10 time steps, here for diffusion, here for advection and here for distributed vs serial.

There is scope to add more tests for the immersed boundary grid, especially for a partial cell bottom, which I believe is still not tested.
I can start adding some tests for it.

@glwagner
Copy link
Member

glwagner commented Nov 7, 2024

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

Good to add tests though I don't know why 100+ time steps matters specifically?

@navidcy
Copy link
Collaborator Author

navidcy commented Nov 7, 2024

Are there any tests that time step a hydrostatic model with an immersed boundary for 100+ time steps?

Good to add tests though I don't know why 100+ time steps matters specifically?

For a regression test, 10 should be plenty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package 📦 Quite meta testing 🧪 Tests get priority in case of emergency evacuation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants