-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
aqua CI and related fixes #389
Conversation
@ChrisRackauckas do you have any idea why when testing locally on my Mac I only get 8 ambiguities but CI gets 35? Is this platform dependent? |
I fixed the ambiguities outside of the spatial solver with the exception of the |
Given the wonkiness of ambiguities reported by Aqua (i.e. being platform dependent for example), and its seemingly over aggressive declaration of things as ambiguous I'm going to close this PR. Some of the fixes might make sense to put in a followup PR, but I don't think we want to add Aqua to the test suite based on what I was observing in this PR. Should SciML decide to add Aqua to CI across the board we can resurrect this. |
I'll keep the branch around so this can possibly be resurrected in the future. |
Adds Aqua test, seems to indicate that #384 is fixed.
@Vilin97 the Aqua test is giving a bunch of method ambiguities coming from the spatial mass action jump constructors. I think the issue is that you have overlapping definitions because of the union type you are using (i.e. multiple constructors cover the case that an input is type
Nothing
).We can fix those in a separate PR, for now I just capped the method ambiguities at 8.