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

update to latest Symbolics (v6) and SymbolicUtils (v3) #345

Closed
isaacsas opened this issue Aug 16, 2024 · 12 comments
Closed

update to latest Symbolics (v6) and SymbolicUtils (v3) #345

isaacsas opened this issue Aug 16, 2024 · 12 comments

Comments

@isaacsas
Copy link
Member

If we could get a release updating these it would be great -- they are holding us back in Catalyst from being able to fully update to them (and ModelingToolkit has now moved to them with the most recent release).

@ChrisRackauckas
Copy link
Member

I got behind on the testing/release process here, but this repo doesn't use maketerm at all so the update should just be a version bump.

@isaacsas
Copy link
Member Author

There is also the issue that arguments is no longer sorted, which we did need to address in several places in Catalyst.

@pogudingleb
Copy link
Collaborator

I will take a look at this over the weekend

@pogudingleb pogudingleb mentioned this issue Aug 17, 2024
3 tasks
@pogudingleb
Copy link
Collaborator

The tests pass with no problems but I am a bit worries about Sam's remark that arguments a no longer sorted. Is this so for any calls (one should be able to tell a - b from a + b)?
Is there any list of breaking changes of these new releases?

@ChrisRackauckas
Copy link
Member

a-b of course would have to keep them sorted because it's not commutative, it's only natively commutative storages which are unsorted and thus don't perform a lexicographical sort upon access.

@pogudingleb
Copy link
Collaborator

a-b of course would have to keep them sorted because it's not commutative, it's only natively commutative storages which are unsorted and thus don't perform a lexicographical sort upon access.

Got it, thanks. Then it should be fine, I plan to merge and make a release soon

@ChrisRackauckas
Copy link
Member

Yes it really only makes a difference to printing. I'm not sure why it was brought up in the context here since symbolic analyses shouldn't have any difference (which is the reason for not sorting by default because it was about 99% of the time)

@isaacsas
Copy link
Member Author

We had several places where Catalyst was interfacing with other packages and it seemed to matter from updates @TorkelE needed to make, so I figured it was worth mentioning.

@ChrisRackauckas
Copy link
Member

It matters for display and DSLs because those deal with the outside world and read in / write out terms. But almost all symbolic passes can assume +,*, etc. are commutative, in which case they don't need to care about argument order. This is why the BasicSymbolic has Add,Pow, etc. forms, since in its manipulations it can specialize a lot by using hash tables for those operations. The order that then comes out of those is then always made up, because a dictionary is inherently an unordered object, so arguments(ex) would have to take the keys(hashtable) and then run a sort! on it to re-order them. However, since any math doesn't actually care about the difference between x + y + z and x + z + y, any math doesn't need to run that sort! operation.

It's just things like printing, since then if you get random ordering users will think something is broken when in reality, it would be better to just say +(Set[x,y,z])or something that basically says "this really is unordered and we did not even store the ordering, so whatever order we spit is completely random". The way to make it not look completely random is to just always sort is lexicographically: users interpret it as "it performed a simplification at that step", when in reality no simplification actually necessarily occurred (though because it's just a hash table [x =>1 ,y=>1,z=>1], + between sets will automatically simplify), it's just that there is actually no stored ordering so if we want it to look like there's any order to it, we just always have to sort.

However, it was found that this sort! was literally 99% of the profile in most operations, and thus for almost all real symbolic stuff it was the most expensive part even though it was unnecessary. So it was moved to a display operation.

@ChrisRackauckas
Copy link
Member

And I will say that some of the sorting added back into Catalyst was actually unnecessary. For example, I think there was a case that was doing it for comparison between objects having a "change". It's really that comparison between two BasicSymbolics should be looking at the subtype of the BasicSymbolic, i.e. which form it is in the unityper, and if it's in this form it's equality between the dictionaries. One way to enforce you have equality is to write the term as a tree structure graph that it represents and then check the terms are the same (which then requires that everything is sorted so you get the same structure), but clearly from this description you could also just check that the keys match and the values match. I think I saw some of that in there. That kind of stuff isn't necessary to police though, if we need to optimize that part of the code I can come around and make a sort-free version, but I assume for DSLs people will probably overuse sorting as it makes things a bit easier than that's okay.

But symbolics passes should almost certainly get away without any sorting, and so the defaulting choice is to force things down the path of no sorting (since it's a lot faster) and if things aren't working for someone give them a sorting option. That sorting option is then a major performance red flag that is easy to audit for and fix, for someone who knows how to deal with this kind of thing, which then makes it rather easy to push the ecosystem down a path where we get a 100x performance benefit.

@pogudingleb
Copy link
Collaborator

pogudingleb commented Aug 19, 2024

I have merged the PR with bumping reps but I got some precompilation errors for ModelingToolkit after the merge. I will take a look at them today-ish and hopefully make a new release.

@ChrisRackauckas
Copy link
Member

That's this mess JuliaObjects/ConstructionBase.jl#92, which also effects old versions. I did a release and a ton of backports so hopefully it should be gone within the next half hour.

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

No branches or pull requests

3 participants