Skip to content

Commit

Permalink
Update to general namespace separator
Browse files Browse the repository at this point in the history 
This is the one place where it's used? Thoguh, does this code need to exist? It seems like it's a separate implementation of SII that is a bit buggy, @TorkelE might be best to just replace with SII?
  • Loading branch information
@ChrisRackauckas
ChrisRackauckas authored Jun 13, 2024
1 parent 8f8290c commit 57f8605
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/reactionsystem_conversions.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ function _symbol_to_var(sys, sym)
if hasproperty(sys, sym)
var = getproperty(sys, sym, namespace = false)
else
strs = split(String(sym), "₊") # need to check if this should be split of not!!!
strs = split(String(sym), ModelingToolkit.NAMESPACE_SEPARATOR) # need to check if this should be split of not!!!
if length(strs) > 1
var = getproperty(sys, Symbol(strs[1]), namespace = false)
for str in view(strs, 2:length(strs))
Expand Down

10 comments on commit 57f8605

@TorkelE
Copy link
Member

Choose a reason for hiding this comment

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

There might be an SII version of this. I asked Aayush if there could be a general function that does this in SII, and he kind of agreed but sounded like there was not and he had other stuff he needed to work on? This one is only really used when XProblems are created, as this is the case where Symbols are not accepted. I agree that, when possible (and maybe it already is), this should probably be moved to SII.

@isaacsas
Copy link
Member

Choose a reason for hiding this comment

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

@TorkelE seems like tests are failing now?

@TorkelE
Copy link
Member

Choose a reason for hiding this comment

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

They fixed one of the things we marked as broken on MTK

@isaacsas
Copy link
Member

Choose a reason for hiding this comment

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

@ChrisRackauckas this code has been around since Catalyst 10, significantly predating SII. If SII now provides equivalent functionality I agree we should switch to it, but currently there are a number of places using symmap_to_varmap throughout the code and docs so we'd need to spend some time updating.

@isaacsas
Copy link
Member

Choose a reason for hiding this comment

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

But it would be good to decide if we are making this change before we release Catalyst V14 since it would be breaking to remove symmap_to_varmap since it is a documented API function.

@ChrisRackauckas
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes should definitely remove it for v14. I'll just merge the fix in anyways for posterity. But @AayushSabharwal is on the case for the replacement.

@isaacsas
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good! I think we are both happy to make the change.

@TorkelE
Copy link
Member

Choose a reason for hiding this comment

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

If Aayush is working on functions like this, another function which would be useful is a conversion of a pure array to symbolic, i.e. taking [:X, :Y] (or (:X, :Y)) to [X, Y] (or (X, Y)). This is used in a couple of cases and would be nice to have a SII function for that (a final case, which I think is only relevant when working with outside packages, is the conversion of a ordered array to a sym map, i.e. [1.0, 2.0, 3.0] to [X1 => 1.0, X2 => 2.0, X3 => 3.0]).

@AayushSabharwal
Copy link
Member

Choose a reason for hiding this comment

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

SciML/SymbolicIndexingInterface.jl#84 has an API for turning Symbols to symbolics. MTK will have a better version of the default implementation. Your first requirement would by satisfied by broadcasting name_to_symbolic. Converting an array of values to a symbolic map is only valid for unknowns (and even then not strictly).

@TorkelE
Copy link
Member

Choose a reason for hiding this comment

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

Conversion of arrays of values to symbolic maps only working for unknowns makes sense, agreed.

Please sign in to comment.