-
Notifications
You must be signed in to change notification settings - Fork 15
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
Warn user about precompilation of symbolic units #58
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
Looks good to me! Thanks for tagging me this patch. I hadn't considered the manual constructer either. |
@@ -314,7 +318,19 @@ module SymbolicUnitsParse | |||
|
|||
const UNIT_SYMBOLS_EXIST = Ref{Bool}(false) | |||
const UNIT_SYMBOLS_LOCK = Threads.SpinLock() | |||
function _generate_unit_symbols() | |||
function _generate_unit_symbols(; testing=Val(false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an FYI — the changes in this PR completely address what I observed in #51! I checked out this branch with my AstrophysicalCalculations
package, and found the expected warning. Then I changed my code to use the Quantity(..., SymbolicDimensions; pc=1)
constructor, as instructed by the warning, and my package precompiled as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
The only reason I'm hesitating to merge is I'm not sure there are any cases where you may actually need to run through that function during precompilation. I ran some testing and it seemed to hit that error even without necessarily precompiling a module with it... So maybe we could just leave the warning, but still allow it to execute as normal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran some testing and it seemed to hit that error even without necessarily precompiling a module with it...
That's interesting. I don't know enough about Julia's stages of compilation / precompilation. I think the warning is great; if I saw it before making #51, I would have used the Quantity
constructor instead of filing an issue. Of course, no rush merging anything from my point of view; AstrophysicalCalculations
is a "brush up on physics before graduate school" kind of package, not a serious kind of package. 😉 Plus the constructor is a great solution as-is.
As noted in #51 by @cadojo,
SymbolicDimensions
parsing is not compatible with precompilation because the unit and constant variables are only created at runtime. (The reason for this is that it can take about a second to generate all the symbols. So, to keep startup time small for performant libraries, the symbolic dimensions are generated at runtime.)This PR:
_generate_unit_symbols
is precompilation is active.What do you think @cadojo?
cc @gaurav-arya