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

Consistent naming: plan profile or move profile? #390

Open
rjoomen opened this issue Oct 2, 2023 · 6 comments
Open

Consistent naming: plan profile or move profile? #390

rjoomen opened this issue Oct 2, 2023 · 6 comments

Comments

@rjoomen
Copy link
Contributor

rjoomen commented Oct 2, 2023

What is the correct naming for this type of profile: plan profile or move profile? The naming in the code is inconsistent, see e.g. here.

I'm willing to refactor this everywhere, once I know the correct term.

@Levi-Armstrong
Copy link
Contributor

I would say move. The initial implementation we had plan_instruction and move_instruction, but removed the plan instruction a while back.

@rjoomen
Copy link
Contributor Author

rjoomen commented Oct 6, 2023

But should OMPLPlanProfile, TrajOptPlanProfile etc. then also be renamed to [...]MoveProfile?

@Levi-Armstrong
Copy link
Contributor

@marip8 and @marrts What do you think?

@marip8
Copy link
Contributor

marip8 commented Oct 6, 2023

I think it's still on our roadmap (mental and currently undocumented) to do a refactor of the profiles to provide a common interface that they all should inherit from. I've started this in the feat/ProfileRefactor branch; here (from #267) I've created the three types of interfaces we think generically cover the use-cases of each planner: WaypointProfile, CompositeProfile, and PlannerProfile. What we're discussing here aligns with what I'm calling the WaypointProfile. I think this type of profile more generally applies the configuration of a planner for a MoveInstruction (since it's API takes a MoveInstruction as input), so I would vote for renaming to MoveProfile.

In digging up this information, I noticed that there does not appear to be an issue or discussion about the profile refactor on this repo even though @Levi-Armstrong and I have discussed it quite a bit in conversations in the past. I'll try to collect all of our thoughts about this and put it into a discussion for documentation and discussion

@rjoomen
Copy link
Contributor Author

rjoomen commented Oct 6, 2023

For reference: #260 and linked work: #255 and #367

@rjoomen
Copy link
Contributor Author

rjoomen commented Dec 5, 2024

Here's a friendly heads up. I'm really interested in the profile refactor, having all the profiles serializable and generic would be a nice cleanup of the way the motion planning is configured. And @marip8's PR is already more than 2 years old...

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

3 participants