Skip to content

Improve @u_str implementation to fix downstream precompilation #106

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

Merged
merged 26 commits into from
Jan 12, 2024

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Jan 4, 2024

Fixes #105 which reported precompilation issues.

  • Manual parser for u_str to avoid calling eval (which breaks precompilation)
    • Also implements for us_str
  • Creates the NoDims type so that dimension(1.0) doesn't throw a method error.
  • Allows you to use uconvert in more contexts, like SymbolicDimensions -> SymbolicDimensions

@SymbolicML SymbolicML deleted a comment from sweep-ai bot Jan 4, 2024
@MilesCranmer MilesCranmer changed the title Attempt basic fix to precompilation [WIP] Attempt basic fix to precompilation Jan 4, 2024

This comment was marked as off-topic.

@MilesCranmer
Copy link
Member Author

@YingboMa what do you think? This completely avoids any eval when evaluating u_str.

I also removed the limitation of type requirement. So you can do things like u"(m, s)".

@gaurav-arya are you okay with this? I think we might have to remove that type requirement to fix precompilation working in downstream packages, since evaluating types requires eval.

@MilesCranmer MilesCranmer changed the title [WIP] Attempt basic fix to precompilation Improve @u_str implementation to fix downstream precompilation Jan 4, 2024
@MilesCranmer MilesCranmer changed the base branch from main to built-in-symbolic-units January 4, 2024 23:29
Base automatically changed from built-in-symbolic-units to main January 5, 2024 00:12
@MilesCranmer
Copy link
Member Author

Since this is quite a significant change in the parsing I'm going to sit on this PR for a bit in case I think of anything wrong with it.

@gaurav-arya if you could take a look also that would be amazing

@gaurav-arya gaurav-arya self-requested a review January 10, 2024 01:22
@MilesCranmer
Copy link
Member Author

For backwards compatibility we could have as_quantity injected into the output – then one could still use u"1" to create quantities?

It's not 1:1 with Unitful then but at least it preserves the type stability. Like q = u"1"; q *= 1.5u"m" for example.

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Jan 11, 2024

Okay I convinced myself the other way. I think u_str should always return the same type no matter what – it's just the safest thing to do. Even though macros are expanded during compilation, I can easily see the user encountering type instabilities because of this aforementioned issue, where u"1" returns a different type from u"m/m".

This is unlike Unitful, but is like the current implementation of DynamicQuantities. So with this change it's no longer breaking.

@MilesCranmer
Copy link
Member Author

Woo! 100% test coverage!

Alright we're good now. I'm merging as it's no longer a breaking change. @gaurav-arya if you did have a comment on the behavior of u_str we can always implement the breaking change on a new version later.

@YingboMa please confirm that precompilation works on v0.11.2 and feel free to add to the stdlib now.

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

Successfully merging this pull request may close these issues.

u"Ω" errors in precompilation
1 participant