-
Notifications
You must be signed in to change notification settings - Fork 23
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
Affine Units Functionality #159
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Thanks! Can you walk me through the implementation a bit? How does it work, what are the fields used for, etc? |
And FYI I would like to get things to 100% unit-test coverage if possible (particularly important as I'm unfamiliar with the code). My expectation is that there will be many unexpected errors that will pop out when going through this process. |
I think the Aqua and JET tests will likely take a bit of work to get passing but are also important |
Sure! As for how it works. Almost all of the changes are in "affine_dimensions.jl", which somewhat mirrors the patterns found in "symbolic_dimensions.jl" (or as much as reasonably possible). The basic structure is the AffineDimension <: AbstractAffineDimension <: AbstractDimension which has the following fields:
The
This is essentially what uexpand(affine_quantity) does when it converts things to SI dimensions. The
But if we used
|
Generally speaking, it's not encouraged to do mathematical operations in affine units because of potential ambiguities present in the offset. Promotion rules will attempt to convert affine quantities into basic dimensional (SI) quantities. However, the function "convert" will fail if the offset is not zero (I allow typical multiplication/division operations for affine units with zero offsets in order to make affine unit macros work with compound units like m/s). The affine unit marcro is "ua" and I registered the Celsius and Fahrenheit units. So you could write
Becuase AffineDimensions are more general than Dimensions and SymbolicDimensions, registering a unit will also make it available to the "ua" macro, and I registered all items in UNIT_SYMBOLS and UNIT_VALUES off the bat, so you could write:
This generality is so that you could make a single Type-Stable container (like a dictionary or vector) that represents both Affine and Non-Affine units and easily convert them all to SI or Symbolic units. Note however, compound non-affine units break, becuase offsets cannot be multiplied:
|
Finally, there is a special macro for registering AffineDimensions only, because the regular registration macro doesn't handle offsets. Registration will apply to anything that can be evaluated as a dimension or quantity. For example, you could do:
Generally, when building AffineDimensions, you want to be explicit about your "offset" units. If you use a numeric value, it will assume the same units as "basedim" which could be a quantity.
This may seem odd until we account for the fact that we could register half-meters so
In such a case, it would be "least astonishing" that in the absence of units, the offset is in the same units as the base dimensions because no other units are mentioned. Building AffineDimesnions from other AffineDimensions will use the basedim's offset as a starting point, so you could do something like this:
|
Quick question - why do affine dimensions need to support algebraic operations on non-offset affine dimensions? If it's non-offset, why not just force the user to immediately convert to SI units if they want to use such operations? |
Anyway, I'll start writing the tests for this on Monday, I just wanted to make sure you were onboard with adding this before I put the effort into it. I was pleasantly surprised that this took me less than a week to do; your package has some fairly elegant ways to plug in this feature (although these plug-in points are a bit hard to find). To reiterate, almost all of the changes are in the new "affine_dimensions.jl" file I added. I also had to make some changes to register_units.jl to add automatic registering of any units to the AffineUnits registry, and make a separate macro for registering AffineDimensions only. I do have some questions:
|
The only reason I do this is so that I can parse units like "ua"m/s"" as an Affine Unit. However, I could just as well do an automatic promotion to SI units and then evaluate the compound unit as Affine in the end if that's something you would prefer. |
This is the command I usually run at the root of this repository: julia --startup-file=no --depwarn=yes --threads=auto --code-coverage=user --project=. -e 'using Pkg; Pkg.test(coverage=true)'
julia --startup-file=no --depwarn=yes --threads=auto coverage.jl Then I use coverage gutters in VSCode https://marketplace.visualstudio.com/items?itemName=ryanluker.vscode-coverage-gutters which highlights the missing lines inside the IDE. |
ReentrantLock lets the same thread acquire a lock multiple times and I didn't see any use of that in this context. Also I expect the SpinLock to basically never get hit, it's just for the case where someone actually tries to register two units simultaneously, which I would expect to be exceedingly rare. |
Excellent, thanks! |
ind = get(AFFINE_UNIT_MAPPING, name, INDEX_TYPE(0)) | ||
if !iszero(ind) | ||
olddims = dimension(AFFINE_UNIT_VALUES[ind]) | ||
if (olddims.scale != newdims.scale) || (olddims.offset != newdims.offset) || (olddims.basedim != newdims.basedim) |
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.
Please use the getters (affine_scale, etc.). I also added one for the symbol
As a heads up I'm making some quick changes as this is taking a long time to go back-and-forth. I removed the definition of |
@test psi == ua"psi" | ||
@test psi == u"psi" | ||
@test psig == ua"psig" | ||
@test_throws AffineOffsetError 0*psig == u"Constants.atm" |
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.
Please use single spaces between infix operators. So 0 * psig
@test map_dimensions(-, dimension(ua"m"), dimension(ua"s")) == AffineDimensions(Dimensions(length=1, time=-1)) | ||
@test map_dimensions(Base.Fix1(*,2), dimension(ua"m/s")) == AffineDimensions(Dimensions(length=2, time=-2)) |
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.
Please avoid these kinds of stylistic patterns with multiple spaces to line up the ==
I am worried we are playing whack-a-mole with these issues and I am concerned with how many reviews I have had to do of this PR which is supposed to be for a fairly simple purpose. Maybe the current design of the AffineDimensions is just not compatible with DQ, if we need to keep monkey patching stuff like this? For example, what if a user has their own function that they define for quantities, and, as done throughout the entirety of DynamicQuantities, they simply do a I'm not sure how to solve this though. It seems really difficult... |
The clearest example of this is Like maybe a different approach could be that The downside is that a raw |
Or maybe just have ustrip return the scaled value, and dimension return the base dimension? Then I guess stuff would work out-of-the-box? |
The most "Julian" solution to this is to properly constrain the parameters and define the operation on those dimensions
As long as the op isn't defined for an |
You don't even have to redefine "my_op" for dimensions. You could also simply have a function called
This will also throw errors on AffineDimensions. Or you could just constrain the the quantity dimension types
The key is using the correct level of abstraction. It's just that your current definition of |
I think in this case, it's best to define |
Thanks for the detailed thoughts. Given that DynamicQuantities is already at v1 with a stable API, we can’t ask users to restrict their function definitions to I appreciate the convenience of treating A safer route would be to either ensure |
At this point, I'm not sure what kind of problems this would fix because how you implemented |
Basically what I am saying is that needing to patch unrelated functions like |
I did give some ideas on how to make the API more generic, but it would break the API and require a 2.x release. Maybe this isn't going to work and I'll have to create yet another units package. |
I'm confused. Why wouldn't the ideas above work? These ones:
|
It would technically work but would require a whole separate API for affine units, where the API in Unitful.jl is pretty straightforward. I'm already trying to use this version of the API in some of my codebases with local versions of this repo and having to make separate functions for affine units will just make this package a headache to use. I'm now having to play whack-a-mole to break an API that makes perfect sense. |
So from my end:
|
Ok maybe let’s try 3? Maybe that will help fix some of the issues |
Okay, I'll give it a shot. |
Welp, breaking ustrip really broke a lot of things. I'm at the point where I don't know if I can fix the remaining issues. I managed to get 1.11 working, but I'm having a hard time with the Julia 1.10 implementation, the problem seems to be in utils.jl file around line 358, I'm not sure why Julia 1.10 is breaking here. This part of the code is hard for me to understand so I'm not sure if it's safe to patch. Are you able to look into this one? |
I can take a look. Before that can you try to fix your merged changes from edits? I think some of my edits were erased; stuff like adding |
This adds a new type of Dimension (AffineDimension) that can be used to describe units with offsets (like Celsius and Fahrenheit). Like most other packages, operations on AffineDimensions are forbidden except subtraction. This is done by throwing an error on convert if the offset is zero; operations are allowed if the offset is zero (by using promote/convert to SI units).