Skip to content
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

Person properties cannot be of type float #125

Open
ChiragKumar9 opened this issue Dec 14, 2024 · 1 comment
Open

Person properties cannot be of type float #125

ChiragKumar9 opened this issue Dec 14, 2024 · 1 comment

Comments

@ChiragKumar9
Copy link
Contributor

Currently, the values associated with person properties must implement Hash (in addition to a series of other traits):

type Value: Copy + Debug + PartialEq + Hash;

This means that person properties cannot be a float because f32/f64 do not implement Hash in Rust. Since wanting to store floats as person properties comes up a reasonable amount of times (i.e., time of last infection, natural history parameters potentially, maybe even per-person vaccine efficacy, etc.), we should have some general solution to this problem.

Here are two thoughts:

  1. Declare whether the property will be indexed when defining the person property and accordingly require whether Hash be implemented on the type. Could also automatically call context.index_property(property) for such properties. Naturally enforces that float properties cannot be indexed, which feels like a good choice.
  2. Internally store floats as some other type for which Hash is implemented. I can't really ever think of a case where our floats would be Nan, so potentially store floats as ordered_float::NotNan<f64> from crate ordered_float.

I think both solutions have their pros and cons, and I'm not yet sure which I prefer.

Side note: when making a person property that is a custom type, I have to derive a series of traits for the struct -- Debug, Clone, Copy, PartialEq, etc. Would it be possible to just do something like #[derive(IxaPersonProperty)]/is this a good idea, or would it be hiding important things from the user?

@ChiragKumar9
Copy link
Contributor Author

Discussed IRL: we will use ordered_float. Whether this gets re-exported from ixa or not or ixa creates a new type called IxaFloat remains to be seen.

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

No branches or pull requests

1 participant