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.

Sign up for GitHub

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

Jump to bottom

feat: allow toggling namespacing independently of complete #3497

Merged
merged 7 commits into from
Mar 30, 2025
Merged

feat: allow toggling namespacing independently of complete #3497

merged 7 commits into from
Mar 30, 2025
+343 −44

Conversation

AayushSabharwal
Copy link
Member

SebastianM-C
SebastianM-C reviewed Mar 25, 2025
View reviewed changes
test/analysis_points.jl
Comment on lines +50 to +54
csys = complete(sys)
ap4 = csys.hej.plant_input
@test nameof(ap4) == Symbol(join(["hej", "plant_input"], NAMESPACE_SEPARATOR))
nsys = toggle_namespacing(sys, false)
ap5 = nsys.hej.plant_input
Copy link
Contributor

Choose a reason for hiding this comment

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

Does complete no longer delete analysis points?

Copy link
Member Author

Choose a reason for hiding this comment

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

complete deletes analysis points in the sense that you cannot perform analysis point transformations on a completed system. However, you can still refer to analysis points from a completed system.

@isaacsas
Copy link
Member

So if we have complete in Catalyst just enable namespacing, we should then be able to use ReactionSystems to index into problems, integrators, and solutions instead of indexing with the converted MTK system we generate?

@AayushSabharwal
Copy link
Member Author

enable namespacing

*disable, but yes

@ChrisRackauckas
Copy link
Member

namespacing is the wrong tense? namespaces? @baggepinnen might have opinions

@baggepinnen
Copy link
Contributor

namespacing is the wrong tense? namespaces? @baggepinnen might have opinions

My opinion here is not very strong, but this function does not really toggle any namespace, it toggles the action of appending a namespace in a future call to sys.x. If the action of appending a namespace is referred to as "namespacing" I think the current name is okay. I have a bigger problem with the horrible structural_simplify, but that's a discussion for another place and time 😅

@AayushSabharwal
Copy link
Member Author

Failures are weird, it passes locally.

@AayushSabharwal
Copy link
Member Author

Okay hmm updating reproduces this on master too

@AayushSabharwal
test: fix rootfinding tests
Loading
Loading status checks…
b54b56f
@ChrisRackauckas ChrisRackauckas merged commit fe19d26 into SciML:master Mar 30, 2025
42 of 45 checks passed
@AayushSabharwal AayushSabharwal deleted the as/nonamespace branch March 30, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SebastianM-C SebastianM-C

@ChrisRackauckas ChrisRackauckas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@AayushSabharwal @isaacsas @ChrisRackauckas @baggepinnen @SebastianM-C