fix Catalyst using warning #944
Conversation
|
Thanks! There are also these two which have been around for a while which I really do not know how to fix. Chris suggested that it might have something with DomainSet to do, but I also see it when loading Catalyst normally sometimes, so not sure. |
|
Those aren't Catalyst related, and I don't see them currently. (There are other ones I am seeing from other packages, but we can't handle those.) |
test/runtests.jl
Outdated
| @@ -66,6 +65,7 @@ using SafeTestsets, Test | |||
|
|
|||
| # Tests extensions. | |||
| @time @safetestset "BifurcationKit Extension" begin include("extensions/bifurcation_kit.jl") end | |||
| @time @safetestset "Steady State Stability Computations" begin include("miscellaneous_tests/stability_computation.jl") end | |||
There was a problem hiding this comment.
@TorkelE these tests use the HC extension, so they shouldn't be run before the extension section. They were much too early in the testing before I think.
There was a problem hiding this comment.
It only really used it because I wanted a nice way to compute steady states of systems with multiple systems. I think it make sense to have the extension section only contain tests that actually tests the extensions? If you do not want tests that utilise that code before I could try and come up with another way to compute those steady states.
There was a problem hiding this comment.
At a minimum if we are using solvers like SteadyStateDiffEq I'd put these after the ODE/SDE/jump tests. So maybe after @time @safetestset "Nonlinear and SteadyState System Solving" begin include("simulation_and_solving/solve_nonlinear.jl") end?
There was a problem hiding this comment.
Definitely shouldn't be testing stability before we test solvers though since it uses their results.
There was a problem hiding this comment.
Also though, if it is relying on the extension's results as a way to validate stuff it should probably come after the tests that validate the extension itself.
There was a problem hiding this comment.
Just to add -- I like these tests and agree that comparing with HC is a good idea. This is just about ensuring everything the test uses has been tested before this test is run.
There was a problem hiding this comment.
It is ust impossible to ensure that all tests follow line where everything is introduced only where it is tested (e.g. we are depending on running simulations in most test files, even before we tests that simulations are correct). In principle this might run two superfluous test blocks (if there turns out to be an HC error only caught when that extension is tested), but that seems excessive to move the test lines around?
If you want we can create an additional test block towards the end for "tangential features" of something, but I do think there is something to be gained by grouping related tests together (where admiteldy the common denominator of the misc tests being that they do not really have a common denominator).
There was a problem hiding this comment.
Yes, I agree that it is impossible in general and we use solving throughout as part of various system tests. But in this case this is testing functionality that is clearly downstream of tests for the ability to accurately solve for steady-states. I moved it to its own section so it is no longer grouped as part of the extension tests.
There was a problem hiding this comment.
Sounds good for now.
I think a better argument is that stability (and e.g. serialization) essentially are self-contained features that are less likely to break from changes, and thus should be tested much further back in the pipeline. Meanwhile, the API test maybe should be even earlier than they are.
Generally I do think we should keep the misc category. I think it can be useful for a new users who introduces some feature and tests. In this case they have a default category where they can stick their tests until further notice.
|
Looking at the test warnings the only one I don't know how to fix, or even what is causing it is ┌ Warning: dot() is deprecated, use the non-do-block formI assume it is GraphViz_jll.related, but I can't find where this warning is even created. The remaining warnings are all I believe from packages we depend on. Primarily SteadyStateDiffEq. |
|
I wen through the warnings in the previous PR and couldn't really figure them out either. I did some testing especially on the SteadyState ones, but couldn't reproduce them when I ran them just in a normal script, so not sure exactly what to do. |
|
Did you turn warnings on via the command line flag when you ran your normal script? |
|
I just ran it in a VSCode window. That usually produces warnings from some stuff, but there might definitely be some details I am unaware of to how warnings are generated which affected this. |
|
I think that one needs to start Julia with |
|
yeah, that sounds like something I might have missed, will probably circle back to this at some later point in time equipped with this knowledge. |
Hopefully fixes the warning when calling
using Catalyst.