Closed jedbrown closed 9 months ago
Thanks for the PR! Let me know when I should review in more detail.
I've flagged the key implementation points. I'm curious whether we want all three features (std gives the prior behavior) and the ability to use none. num-traits without default features (just providing FloatCore) is very light. Any suggestions (including simplification) welcome.
Since I don't do much embedded work, I'll let you be the judge of whether having all three new levels is useful. I think if we get the implementation right, having four different options shouldn't bring too much complexity, since they form a strict hierarchy (at least that's what I understand). We can probably define top level macros to make distinguishing which level of the hierarchy we're at easier.
From what I gather, there are no differences in the available float methods between std
and no_std
with float
, is that correct?
For some premature bikeshedding (sorry): What do you think about renaming the float
and float_core
features to num_traits_float
and num_traits_float_core
respectively? It's a bit long and ugly, but from a user perspective, having a float
feature might be confusing, since we also have f32
and f64
features.
On a side note, for later: At some point we'd probably want to make all these anonymous methods on Quantity
part of a implementation of the corresponding Num
trait.
Another thing regarding code quality that just came to mind: Another way of deciding which methods to implement for each float type could be by adding appropriate methods on the StorageType
trait. This could help "contain" the compile features to the storage_types
module.
Not something we/you need to think about right now though, but I might implement this in the future.
From what I gather, there are no differences in the available float methods between
std
andno_std
withfloat
, is that correct?
Correct, those are functionally equivalent, though float
is using num-traits/libm
, which are lower performance implementations.
I'm happy to use more verbose trait names, but would like to see if we can simplify. Adding the float_core
feature (depending on num-traits
) has essentially no effect on build time (0.03 seconds in my attempt to time the difference). The size difference in libdiman.rlib
with --no-default-features --features f64,si
versus adding float_core
is negligible (0.3% in debug, 0.2% in release). Without the si
feature, the rlib size is identical with/without float_core
. So I'm having trouble justifying having the no-float_core
option (which then needs to be tested). Perhaps if someone wants to use Diman for microcontrollers with fixed-point arithmetic, but that would involve more extensive changes.
So we could get rid of the float_core
feature flag and only have num-traits-float
and std
. This still signs us up for managing which num-traits
feature will be used to enable Float
. One way to get out of that business is for us to only depend on num-traits
with no features, but require that the user enable a feature that provides it. At present, any of these would work (and there may be more ways to get it in the future, such as "cuda-libdevice"
):
num-traits = { version = "0.2.17", default-features = true }
num-traits = { version = "0.2.17", default-features = false, features = ["std"] }
num-traits = { version = "0.2.17", default-features = false, features = ["libm"] }
This would also let us get rid of the std
feature in Diman, leaving only num-traits-float
. The downside is that one of these lines needs to go in the user's Cargo.toml and resolve to a compatible version of num-traits
. I also suspect there are non-float things we'd want to know about std
for.
Assuming we don't want the above compromise, I lean toward naming the features std
and num-traits-libm
. If neither are enabled, we use num-traits::float::FloatCore
to provide stuff like abs
and powi
, but won't have sqrt
et al. There is an argument for calling the features simply std
and libm
.
PS: I think using num_traits::Float
would allow some simplifications, and possibly a blanket implementation without needing to handle f32
and f64
separately. I expect that would also reduce compile time since monomorphization should be faster than double the code. I think this is at least related to your suggestions above.
Thanks for the analysis.
Requiring at least FloatCore
seems like a good option for now.
I prefer having the std
feature in diman, even if we don't need explicitly need it right now.
Assuming we don't want the above compromise, I lean toward naming the features std and num-traits-libm.
I like this, I think it is explicit and clear. As for calling it just libm
is there any reason to expect to have to provide other libm
-related things that are not provided through num_traits
? Because if not I'd prefer sticking with num_traits_libm
.
PS: I think using num_traits::Float would allow some simplifications, and possibly a blanket implementation without needing to handle f32 and f64 separately. I expect that would also reduce compile time since monomorphization should be faster than double the code. I think this is at least related to your suggestions above.
Yes, I think it should be possible to do something like
impl<S: Float> Float for Quantity<D, S> {
}
This would require pulling in a num_traits
dependency even if just si
is enabled, but it might be worth doing.
Hmm, libm
has functions like erf
and tgamma
that are not exposed through num-traits
. I don't know if we'd ever want to bind those through an extension trait -- it seems like something that could go upstream into num-traits
(but evidently hasn't yet).
A further advantage of num_traits::Float
is that users can easily write their code to be generic over the scalar type, which is likely to be a common request (I would use it in material models, e.g., computing f64
residuals for Newton but using f32
in the linearization for the solve). The library is light enough to have negligible impact on build times so I don't see a tangible downside to that dependency.
A further advantage of num_traits::Float is that users can easily write their code to be generic over the scalar type, which is likely to be a common request (I would use it in material models, e.g., computing f64 residuals for Newton but using f32 in the linearization for the solve). The library is light enough to have negligible impact on build times so I don't see a tangible downside to that dependency.
I agree - I'm totally fine with it as a dependency.
We might also want to remove the debug impl altogether without std
. The problem is that it requires runtime information about available units which adds some amount of memory. Not sure if this is relevant though.
in the current state, two of the compile_fail tests are failing, since the compiler suddenly calls
example_system::Quantity
justQuantity
.
I have not been able to reproduce any of these failures.
Also, I can confirm Diman no_std
is working inside CUDA kernels now.
This is kinda rough currently, but I've tested it in several modes. There are three new features, which we might want to pare down.
std
(default, all float methods are available)float
(usenum-traits
withlibm
so that all float methods are available inno_std
environments. In the future, I anticipatenum-traits
will have other features to use target-specific math libraries (e.g.,libdevice
for nvptx64-nvidia-cuda).float_core
(usesnum-traits
with minimal features -- provides some basic float methods, but not the numerical math functions)There are a couple hiccups when none of these features are available -- I'll comment on those shortly.