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

Support Symbolic Arrays of arbitrary length #3107

Merged
merged 11 commits into from
Oct 11, 2024

Conversation

ven-k
Copy link
Member

@ven-k ven-k commented Oct 8, 2024

Checklist

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

Additional context

Following is now supported:

@mtkmodel ModelWithArrays begin
    @structural_parameters begin
        N = 2
        M = 3
    end
    @parameters begin
        p1[1:4]
        p2[1:N]
        p3[1:N, 1:M] = 10, [description = "A multi-dimensional array of arbitrary length with description"]
        (p4[1:N, 1:M] = 10), [description = "An alternate syntax for p3 to match the syntax of vanilla parameters macro"]
    end
    @variables begin
        v1(t)[1:2] = 10, [description = "An array of variable `v1`"]
        v2(t)[1:3] = [1, 2, 3]
    end
end

A dedicated section, in the docs, describes it.

@ven-k ven-k force-pushed the vkb/array-length-as-input branch 3 times, most recently from cdc3d69 to 5239e4f Compare October 8, 2024 18:24
@ven-k ven-k marked this pull request as ready for review October 8, 2024 18:28
@ven-k
Copy link
Member Author

ven-k commented Oct 9, 2024

The tests pertaining to Model-parsing (model_parsing and in dq_units and units)have passed. The failure looks unrelated.

Update metadata dict and kwargs for symbolic arrays
- Ensure `@variables` are variables of an independent var.
- In both `@variables` and `@parameters` case, ensure a single indpendent var is used
With many ways to define, this feature demands a dedicated section.
While here, removes passing `indices` around to generate_var, update_kwargs_and_metadata! and parse_variable_def! as these no longer handles symbolic-arrays.
@ven-k
Copy link
Member Author

ven-k commented Oct 11, 2024

This PR is rebased off the master and NO_VALUE changes are included.

@@ -278,6 +279,21 @@ end
@test MockModel.structure[:defaults] == Dict(:n => 1.0, :n2 => "g()")
end

@testset "Arrays using vanilla-@variable syntax" begin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the test using structural parameters

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parameters begin
p1[1:4]
p2[1:N]
p3[1:N, 1:M] = 10,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this one substantially more difficult to support?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really.
Parser is now more granular. It picks up relevant bits of expression at the parsing step.
I've tried to batch similar ones together (example) and code is in a that is simpler to maintain.

Actually, let me add doc-strings to each parser step to indicate the syntax it parses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the doc strings matching the block here: 4c0b1ed

@ven-k
Copy link
Member Author

ven-k commented Oct 11, 2024

The doc failure is unrelated (errors stem from initialization.md)

@ChrisRackauckas ChrisRackauckas merged commit 535d1f4 into SciML:master Oct 11, 2024
19 of 25 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