molpopgen / demes-rs

rust tools for the demes ecosystem
MIT License
0 stars 1 forks source link

Arithmetic on low-level types #277

Closed molpopgen closed 1 year ago

molpopgen commented 1 year ago

The various new types could use implementations for several traits from std::ops.

There's a good bit of boiler-plate to this, as we need symmetry for all non-Self types involved in the ops. (We definitely want f64 supported.)

Because subtracting two, say, Time values can lead to a negative number, it seems that the output of such ops should be f64.

alxsimon commented 1 year ago

from this SO questions, maybe several ways of shortening the boilerplate?

molpopgen commented 1 year ago

That latter two crates may be unmaintained (no releases for a very long time).

The "trick" that they don't handle (based on my scan of their docs) is that we don't want the output to be Self. Rather, we always want f64. The reason is that Time(5) - Time(10) is not a valid Time, since Time must be non-negative. But -5 is a valid "time delta". The path of least resistance there is to have the output time be f64.

alxsimon commented 1 year ago

I might try to come back to this in the near future. What do you mean by symmetry in your first post?

Also what's the list of new types to do?

and traits to implement?

molpopgen commented 1 year ago

Symmetry refers to the operation A + B is not the same as B + A, where A,B are types. So, if you implement Add to give A+B, you need to also implement so that B + A works.

I think one or both of those two types are the place to start.

alxsimon commented 1 year ago

ok so if I understand correctly for instance for Time you need: Time + Time Time + f64 f64 + Time

molpopgen commented 1 year ago

Yes, I think that is correct.

molpopgen commented 1 year ago

I think there's some subtlety: Time + Time could return Time, but Time - Time should return f64 because an invalid (negative) Time could result. So does one always want to return f64 for API consistency?

alxsimon commented 1 year ago

I'd lean towards f64 everywhere for consistency, but you know the use cases better than me.

alxsimon commented 1 year ago

Is there a problem in implementing this in the impl_newtype_traits! macro, feels like there the same implementation can be used for all newtypes. But I'm probably missing something if you haven't done it this way yet?

alxsimon commented 1 year ago

I'm going to list the newtypes here just to remember them:

alxsimon commented 1 year ago

Other question, do you absolutely want the traits for direct arithmetics with f64? (Time + f64) This simplifies the api but you could also require that you need always have Time + Time::from(f64) so that the second time is checked to be valid time (e.g. you might not want it to be negative)

molpopgen commented 1 year ago

Is there a problem in implementing this in the impl_newtype_traits! macro, feels like there the same implementation can be used for all newtypes. But I'm probably missing something if you haven't done it this way yet?

Ultimately, a macro will be the way to go. I didn't do it b/c I didn't feel the need for these operations.

molpopgen commented 1 year ago

One idea to use the num crate and implement traits like checked_add: https://docs.rs/num/latest/num/trait.CheckedAdd.html. Likewise CheckedSub, etc./

This way, Time(5) - Time(10) would return on None.

The potential issue here is that these traits are intended to provide safe arithmetic and not (necessarily??) to constrain values to be strictly positive. I worry that the implementation of Sub for Self, Self would be expected to wrap around, which is not the natural operation for the underlying type.

molpopgen commented 1 year ago

Oh, Sub et al have an Output associated type. That type can be Option so that invalid values can return None. That helps a lot for the Self,Self case.

alxsimon commented 1 year ago

Ultimately, a macro will be the way to go. I didn't do it b/c I didn't feel the need for these operations.

Yeah I had already started implementing a new impl_newtype_arithmetics! macro, but I'll wait for us to discuss in more detail the final technical specs you want to have.

Oh, Sub et al have an Output associated type. That type can be Option so that invalid values can return None. That helps a lot for the Self,Self case.

Yes that's the other option I guess is returning the newtype on operations with any type, which would make sense to me. If you're adding any number to a DemeSize, I'd expect to get a DemeSize in return?

molpopgen commented 1 year ago

Here's a mockup - playground.

molpopgen commented 1 year ago

Yes that's the other option I guess is returning the newtype on operations with any type, which would make sense to me. If you're adding any number to a DemeSize, I'd expect to get a DemeSize in return?

I agree. There's little reason, IMO, to be returning f64 here. And if anyone needed that conversion, we can provide From

molpopgen commented 1 year ago

Updated mockup.

This is the simplest that I can come up with.

For this to be useful here, our newtypes all need to implement From for f64, which isn't a bad idea anyways.

EDIT: you can simplify it further: playground.

molpopgen commented 1 year ago

Note: the newtypes do impl From for f64...