Skip to content

Conversation

@mandli
Copy link
Member

@mandli mandli commented Aug 26, 2025

This PR adds a testing matrix so we can test multiple platforms, compilers, and build types. This includes

  • Ubuntu and macos
  • compilers (GCC only right now)
  • a debug and optimized compiler build
  • different versions of python

Most of these are more place-holders as we suss out, which ones work and we expand from there.

This PR also adds archiving of test results, although it currently does not archive anything.

@mandli mandli marked this pull request as draft August 26, 2025 12:57
@mandli mandli requested a review from ketch August 26, 2025 12:57
@mandli mandli self-assigned this Aug 26, 2025
@mandli
Copy link
Member Author

mandli commented Aug 26, 2025

I noticed that numpy < 2.0 is currently installed. Do we want to modify this, or test against multiple versions of numpy?

@coveralls
Copy link

coveralls commented Aug 26, 2025

Coverage Status

coverage: 46.627% (+0.01%) from 46.613%
when pulling 86c212b on mandli:add-testing-matrix
into de24cc8 on clawpack:master.

@mandli
Copy link
Member Author

mandli commented Aug 26, 2025

I also added pyflake check, but it is failing. I think it is probably a good idea to keep the pyflake calls, but I will have to go in and fix the errors.

@mandli
Copy link
Member Author

mandli commented Aug 26, 2025

I corrected some of the errors from pyflake. Some comments:

  • There is a script cleanup_examples.py that I think could easily be removed or replaced with something simpler.
  • I modified fvmbook/chap22/inclusion/inclusion.py, but did not test it specifically.
  • The fileio.netcdf contains a lot of the errors, but I noticed it is woefully out of date. I am tempted to just disable it and add NotImplemented exceptions, but wanted to note here if I was to do that, or if there is a strong desire for this.

@mandli
Copy link
Member Author

mandli commented Aug 26, 2025

The tests are failing now because of the following error at build time:

fatal error: threads.h: No such file or directory

It may have something to do with this issue, but it is unclear. Going to try using the latest numpy to see if that helps.

@mandli
Copy link
Member Author

mandli commented Aug 26, 2025

So at least the tests are consistent now, failing with test_acoustics_3d.py during the run (test is not failing itself). Not sure why this is occurring, but getting the output from the failure may be a good impetus for getting the archiving working.

@ketch
Copy link
Member

ketch commented Sep 3, 2025

Thanks for getting this all set up and working through some version issues!

So if I understand correctly the tests all pass with "opt" compiler flags, but not with "debug" compiler flags.

Do you know why we don't get any output from the failed test? And why it doesn't perform the remaining tests?

@mandli
Copy link
Member Author

mandli commented Sep 4, 2025

We need to upload the errors that seem to be appearing for the debug flags. Clearly we should upload the failure text, although it is odd that the interface does not report more. I am trying to reproduce this locally and will report back.

The other failure has to do with coveralls and my best guess is that we are trying to upload more than once to coveralls and it does not like that. We can easily pick one of the matrix to upload from, maybe the ubuntu-latest gcc opt py 3.12 run?

@mandli
Copy link
Member Author

mandli commented Sep 4, 2025

I think I unbroke things, but the debug cases are still failing and because PyTest is not completing we are not getting any of the output from what I added (although I am not confident that is working in any case).

@mandli
Copy link
Member Author

mandli commented Sep 5, 2025

@ketch Not catching the output seems to have given some more indication of what's going wrong. I looked into it briefly, but did not immediatley see what's going wrong, or if the failure is really not there and something else is causing it. At least this may be a place to start.

@ketch
Copy link
Member

ketch commented Sep 7, 2025

Wow, this has actually uncovered a significant bug! One that has been in my code probably since I wrote it back during my PhD. Not sure how this didn't cause problems before.

It's a rather annoying issue: in SharpClaw, there are different options for limiting -- you can limit based on waves, or based on conserved quantities. But that means that the size of the mthlim array should sometimes be num_waves and sometimes num_eqn.

I guess the right thing would be to make this array get resized based on which kind of limiting is selected, but that seems awkward and the truth is we almost never use the flexibility of applying different limiters to different fields. So I'm tempted to just say that if you limit conserved quantities, then you can only choose one limiter (i.e. mthlim(1)) and it will get applied to all of them. What do you think @mandli?

@mandli
Copy link
Member Author

mandli commented Sep 7, 2025

I guess my thought is to make mthlim be max(num_eqn, num_waves) and check at some point (once) that it is the right size. This would be minimally invasive and I think easy to implement.

I am trying to think though, if you had multiple waves for one field say, without having the rest of the limiters indexed by wave, I am not sure how you would distinguish that. Not that you would not be able to do something, we just do not support that currently. Curious what you see as being the full functionality possible though.

@ketch
Copy link
Member

ketch commented Sep 8, 2025

I guess my thought is to make mthlim be max(num_eqn, num_waves) and check at some point (once) that it is the right size. This would be minimally invasive and I think easy to implement.

What do you mean by "right size"? Just max(num_eqn, num_waves)? Or exactly the size that will be used based on the limiting method chosen? The first one would be easy to implement, and the only caveat is that some entries of mthlim would sometimes be ignored. I think that approach should be fine.

I am trying to think though, if you had multiple waves for one field say, without having the rest of the limiters indexed by wave, I am not sure how you would distinguish that. Not that you would not be able to do something, we just do not support that currently. Curious what you see as being the full functionality possible though.

I don't understand what you're saying...

@mandli
Copy link
Member Author

mandli commented Sep 8, 2025

What do you mean by "right size"? Just max(num_eqn, num_waves)? Or exactly the size that will be used based on the limiting method chosen? The first one would be easy to implement, and the only caveat is that some entries of mthlim would sometimes be ignored. I think that approach should be fine.

Yes, exactly this. We could stick NaNs in the unused indices just to make sure they are not used.

I don't understand what you're saying...

I was thinking that you were implying that you could do limiting on some of the waves and some of conservative quantites, mixing which you were limiting. If there were more waves than equations though this would seem to be a bit tricky unless the waves were associated with one of the equations. The opposite with more equations then waves also. Mostly this would make sanity checking complex, or maybe we would just ignore making sure a user was not breaking their own code.

@ketch
Copy link
Member

ketch commented Sep 8, 2025

Yes, exactly this. We could stick NaNs in the unused indices just to make sure they are not used.

Okay, I'll implement that in a PR tomorrow.

I was thinking that you were implying that you could do limiting on some of the waves and some of conservative quantites, mixing which you were limiting. If there were more waves than equations though this would seem to be a bit tricky unless the waves were associated with one of the equations. The opposite with more equations then waves also. Mostly this would make sanity checking complex, or maybe we would just ignore making sure a user was not breaking their own code.

Oh no, we don't do that and I don't want to go there. It's either all waves or all conserved quantities.

@mandli
Copy link
Member Author

mandli commented Sep 8, 2025

Oh no, we don't do that and I don't want to go there. It's either all waves or all conserved quantities.

Everytime I start thinking about the complexities of doing so I understand why it's best left alone.

@mandli
Copy link
Member Author

mandli commented Oct 6, 2025

@ketch made some bug fixes and we identified a long standing "bug" so it looks like we have one more thing to work out with the 3D acoustics test. Also need to turn off the debugging message that was added.

@mandli mandli marked this pull request as ready for review October 6, 2025 22:17
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