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

move updating measurements into new method #746

Conversation

firemark
Copy link
Contributor

For my cases I have written script to compare many localization estimators outside the ROS.

For my usage I need only method to update filter with specified time, but this logic is inside periodicUpdate method, so I extracted into new method.

Copy link
Collaborator

@ayrton04 ayrton04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I am hesitant to merge something that seems like it is intended to solve one user's needs, this shouldn't affect the behavior or performance of the package. I'll ask @SteveMacenski to weigh in as well.

@SteveMacenski
Copy link
Collaborator

SteveMacenski commented May 3, 2022

Galactic goes EOL in 6 months, so really I don't care very much, but if we wanted to make this change across the board, we should do so across the board so that we don't revisit this every new distribution. Looking at the function periodicUpdate, I think it should be broken out into some smaller functional units so this would be in the right direction, but I wouldn't merge as is with just doing 1/10th of the work needed to accomplish it.

So I suppose in its current form, my thought would be "no", but if a well done separation of functional code in periodicUpdate was done to break out individual operations to make the code more self documenting, I could support merging that total change.

Although, this is a long term stable package that isn't really getting many updates, so I am susceptible to the "don't fix what ain't broke" argument to keep maintainer overhead as low as possible.

So my answer is a tentative 'no' unless the user wants to patch up the entire function, and even then subject to @ayrton04's appetite for a large change.

@firemark
Copy link
Contributor Author

I decided to close this pull request and created new one: #761

@firemark firemark closed this Jun 28, 2022
@firemark firemark deleted the move-updating-measurements-into-new-method branch June 28, 2022 10:16
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

Successfully merging this pull request may close these issues.

3 participants