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

Make coverage_analyzer.py support Python 3 #680

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

trey0
Copy link
Contributor

@trey0 trey0 commented Feb 15, 2023

Usually when implementing Python 3 support for a script, you test the Python 2 version under Python 3, get some silly compile errors, fix them, and then the fixed version has identical output under both versions.

In this case, I never got any compile errors under Python 3 (great!) but the results subtly disagreed between the two versions for reasons that I wasn't able to track down (awful).

So what I ended up doing was a fairly substantial refactoring of the code to simplify it in ways that make the behavior easier to understand and predict. As a bonus, the new version runs about 100x faster! In the single real-world test case I got from @amoravargas, it produces the same output as the old version under both Python 2 and 3.

@amoravargas please review (I would have tagged you as a formal reviewer but you were not in the list of options).

@trey0 trey0 requested a review from bcoltin February 15, 2023 21:30
@bcoltin
Copy link
Member

bcoltin commented Feb 15, 2023

Looks fine to me but will wait for feedback from Andres

@amoravargas
Copy link
Collaborator

Looks good. Thanks for the improvements.

@bcoltin bcoltin merged commit cb11b6a into nasa:develop Feb 16, 2023
marinagmoreira added a commit that referenced this pull request Mar 9, 2023
* Documentation update (#668)

* docker docs update

* make build instructions ROS1

* updating install instructions, hopefully making them easier to follow

* updated from github PR reviews

* fix typo

* replace list with append (#671)

* ci: bump to checkout@v3 (#673)

* Unlatching topics (#674)

* Changed heartbeat topic to be unlatched.

* Changed echoed command to not latching.

* no subscribers and info is obtained by service, so no latched topic needed

* removing latched on frequently published topics

* Made command topic to not atching.

* no need for these to be latched

* sub and pub in same node, no need to latch

* no need to be latched no subs and not recorded, rviz should be opened when published

* Made ground bridge topics not latched.

* Fixed heartbeat topic latched comment.

---------

Co-authored-by: Katie Browne <kathryn.browne@nasa.gov>

* support multiple versions for docs (#675)

* Make coverage_analyzer.py support Python 3 (#680)

Refactor to simplify and make output identical between Python 2 and 3

* Release 0.17.0 (#683)

* add missing glog flag that caused problems in some systems (#685)

* Make isort repeatable (#694)

* make isort repeatable

* isort now consistently groups astrobee imports as first party; one-time update to apply to old files

* moving ff_names to ff_common, updating headers (#696)

---------

Co-authored-by: Trey Smith <trey.smith@nasa.gov>
Co-authored-by: Katie Browne <kathryn.browne@nasa.gov>
luisa-mao pushed a commit to luisa-mao/astrobee-1 that referenced this pull request Jul 11, 2023
Refactor to simplify and make output identical between Python 2 and 3
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