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

Bikesheding of quantity_point::relative() #479

Closed
mpusz opened this issue Aug 4, 2023 · 6 comments
Closed

Bikesheding of quantity_point::relative() #479

mpusz opened this issue Aug 4, 2023 · 6 comments
Labels
design Design-related discussion

Comments

@mpusz
Copy link
Owner

mpusz commented Aug 4, 2023

quantity_point::relative() is considered "unsafe" as discussed in #414 (comment) but also really needed as mentioned in #414 (comment).

We can discourage its use by making the name longer 😉

One of the proposals was to name it relative_to_origin(), but after a longer thought, I am not sure if the "relative" is the correct name here. We are used to it because it is n mp-units "forever", but it might not be a correct term in the domain.

Even our documentation says explicitly that the affine space is about points and vectors where we model a vector with the quantity class template. So another candidate could be vector_from_origin. This, however, might be also unfortunate as we also support vector and tensor quantity characters as documented here: https://mpusz.github.io/mp-units/2.0/users_guide/framework_basics/character_of_a_quantity and those names would collide and make it hard to understand what kind of "vector" we talk in each context.

So maybe it should be just called quantity_from_origin()?

Wikipedia article https://en.wikipedia.org/wiki/Affine_space also mentions "displacement vectors, also called translation vectors or simply translations" so maybe a translation_from_origin() is a good name here.

Any thoughts?

@mpusz mpusz added the design Design-related discussion label Aug 4, 2023
@JohelEGP
Copy link
Collaborator

JohelEGP commented Aug 4, 2023

quantity_from_origin is a nice generalization from
std::chrono::time_point::time_since_epoch.

@mpusz
Copy link
Owner Author

mpusz commented Aug 4, 2023

Yes, and it nicely complements quantity_point::point_from(PointOrigin).

@burnpanck
Copy link
Contributor

I think this should be resolved in analogy to #476: Instead of the unsafe .relative(), we could have a save.relative_to(PointOrigin) that returns an rvalue, as-well as a .stored_quantity_relative_to(PointOrigin) (name of course still to be bike-shedded) that either returns an appropriate lvalue reference or fails to compile.

@JohelEGP
Copy link
Collaborator

JohelEGP commented Aug 4, 2023

So this is really #477,
but for quantity_point's stored quantity.

@chiphogg
Copy link
Collaborator

chiphogg commented Aug 5, 2023

Some thoughts so far:

  • I agree we should avoid using the vector word in this context, for the reasons stated.
  • I think displacement_from or translation_from seem like good choices.
  • The only thing this function gives us that we can't get from point-point subtraction is the ability to get a reference to the stored value, right?

@JohelEGP
Copy link
Collaborator

JohelEGP commented Aug 6, 2023

"Offset" is another alternative to the overloaded "vector".
Or maybe it doesn't matter,
since they're at different levels of abstraction.
It could be that giving them different names is worse than calling them by what they are.

@mpusz mpusz closed this as completed in 07ce64d Aug 18, 2023
mpusz added a commit that referenced this issue Aug 27, 2023
…ntity_point::quantity_ref_from(PO)`

Resolves #479 and relates to #477
mpusz added a commit that referenced this issue Sep 11, 2023
…ntity_point::quantity_ref_from(PO)`

Resolves #479 and relates to #477
mpusz added a commit that referenced this issue Sep 13, 2023
…ntity_point::quantity_ref_from(PO)`

Resolves #479 and relates to #477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design-related discussion
Projects
None yet
Development

No branches or pull requests

4 participants