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

Parser improvements #22

Merged
merged 39 commits into from
Jul 22, 2024
Merged

Parser improvements #22

merged 39 commits into from
Jul 22, 2024

Conversation

jClugstor
Copy link
Collaborator

Checklist

  • [x ] Appropriate tests were added
  • [x ] Any code changes were done in a way that does not break public API
  • [ x] All documentation related to code changes were updated
  • [ x] The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • [x ] Any new documentation only uses public API

Additional context

Add any other context about the problem here.

This PR adds proper support for parsing in to an AST, and evaluating that AST to get a ModelingToolkit system. This means that the parsing no longer relies on parsing Julia expressions in to symbolics expression, adds proper support for all BaseModelica operators, etc.

These changes were necessary to make implementing all of the BaseModelica specification possible in the future.

@ChrisRackauckas
Copy link
Member

Precompilation failure?

@jClugstor
Copy link
Collaborator Author

Yeah, not sure why, looks like MTK can't precompile

ERROR: LoadError: `unsorted_arguments(x)` is deprecated, use `arguments(x)` instead.

@jClugstor
Copy link
Collaborator Author

I found that, but isn't that just where the deprecation is set? How would I turn using istree in to an error, isn't that what depwarn=error does? Wouldn't the warning be coming from wherever istree is called, which is what I want to find?

@ChrisRackauckas
Copy link
Member

just delete that line. Make it impossible to get a deprecation

@jClugstor
Copy link
Collaborator Author

Oh yeah I tried that, all the tests pass with out any errors.

@jClugstor
Copy link
Collaborator Author

jClugstor commented Jun 28, 2024

I know what's happening. Basically the ambiguities test for aqua creates a new Julia instance, and it uses Base.julia_cmd() to do it. So any options that were used to start the original Julia instance would be applied to it.

# Ambiguity test is run inside a clean process.
    # https://github.com/JuliaLang/julia/issues/28804
    code = """
    $(Base.load_path_setup_code())
    using Aqua
    Aqua.test_ambiguities_impl(
        $packages_repr,
        $options_repr,
        $exclude_repr,
        $skipdetails,
    ) || exit(1)
    """
    cmd = Base.julia_cmd()
    if something(color, Base.JLOptions().color == 1)
        cmd = `$cmd --color=yes`
    end
    cmd = `$cmd --startup-file=no -e $code`

When I precompile MTK in a Julia session with depwarn=error I get a warning about how istree is deprecated, even though MTK doesn't use istree anymore. I get a warning, not an error.

I think when Aqua gets to the point of using ModelingToolkit in BaseModelica, the istree warning is triggering an error.

I'll try to find where this warning is coming from and what's causing it.

@ChrisRackauckas
Copy link
Member

Just throw a testset over the tests for now so it runs them all. See ModelingToolkit's test form for that.

@jClugstor
Copy link
Collaborator Author

They're all in testsets and safetestsets. Just the Aqua tests fail.

@ChrisRackauckas
Copy link
Member

testset over that

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Jun 29, 2024

Just do exactly as the ModelingToolkit tests...

@jClugstor
Copy link
Collaborator Author

Actions was down for a bit. Yeah, the GROUP stuff, right?

@ChrisRackauckas
Copy link
Member

@jClugstor
Copy link
Collaborator Author

It decides to work now. Simply lovely

@ChrisRackauckas
Copy link
Member

You're not running anything if you haven't set any test groups.

@jClugstor
Copy link
Collaborator Author

@jClugstor
Copy link
Collaborator Author

It's actually running the tests now, I had to add with: group: "${{ matrix.group }}" to make sure it actually runs the groups :-|

@jClugstor
Copy link
Collaborator Author

Is it ok to merge this?

@ChrisRackauckas
Copy link
Member

Run the formatter.

@jClugstor jClugstor merged commit bc03c37 into SciML:main Jul 22, 2024
9 checks passed
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.

2 participants