-
Notifications
You must be signed in to change notification settings - Fork 52
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
Split cubic & ideal EOS (and common backbone) into separate package(s) #106
Comments
Splitting Clapeyron is an idea we've been thinking about for a while. This is mainly because some of the features we've been developing are getting quite niche and often warrant a package of their own. I'm still of the mindset that it's nice to have one unified package. However, as Clapeyron grows, it becomes more apparent that we should split it up. That being said, I doubt SAFT represents a large overhead in Clapeyron? Surely something like our database parsers and methods should contribute much more. I might be wrong though. |
on limited testing:
So, the main overhead seems to be the code before the EoS (parsing and methods) on how to split the package, the main future solution will almost surely be something like what they do in https://github.com/JuliaArrays/ArrayInterface.jl , but we still need to be the proper hierarchy of packages to load. |
with julia 1.8, the new macro
worst offenders:
i'm following https://sciml.ai/news/2022/09/21/compile_time/ btw. so it seems that most of our first time to compile is due to CSV |
Chris’s blog post today mentioned changing import order to put overload-heavy packages like StaticArrays later to reduce the number of methods that need to be checked when loading other dependencies. I haven’t played around with import order, and it’s a bit painful to trawl through dependency trees to figure out where method overloads are coming from, but moving StaticArrays down the list would be an easy first thing to try. |
yes, i'm playing with the package loading order to see the most efficient way. i found one nasty invalidation when loading any JLL after StaticArrays:
|
Wild! Rmath_jll load time is cut from 405 to 5 ms on my machine with the load order reversed. I didn't expect such a large impact. |
There are two types of work to be done:
One quick way to reduce package loading by half would be to remove CSV.jl and just use DelimitedFiles.jl? We use the Tables.jl interface to iterate over the parsed values, so anything Tables.jl compliant could work |
One idea is to bypass the table parsing directly and pass the params yourself:
That would skip the compile first time of the DB parser completely, at the cost of needing to specify your params yourself |
I wonder if we should allow users to define parameters manually for all our models. That seems to be the way all other packages like ours work. Ive always found that really tedious but it's definitely easier than the csvs |
The complexity of SAFT equations of state imposes quite a lot of overhead on this package, and that’s entirely justified for a research-grade EOS, but for simple day-to-day usage, it’d be great to have a low-overhead, fast-loading EOS package covering ideal/SRK/Peng-Robinson.
The text was updated successfully, but these errors were encountered: