fix: apply the documented coordinate rounding to all outputs, not just the API - #1385
Open
NoiceHax wants to merge 1 commit into
Open
fix: apply the documented coordinate rounding to all outputs, not just the API#1385NoiceHax wants to merge 1 commit into
NoiceHax wants to merge 1 commit into
Conversation
The output reference says latitude and longitude are written with reduced precision for privacy, but the rounding only happened in the API client. emissions.csv and the other output methods got the full precision values straight from the geolocation lookup. Round once in _ensure_geo_metadata, where the coordinates are stored in the config, so all output methods are consistent. self._geo keeps the full precision values, which the Electricity Maps carbon intensity lookup needs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The output reference says the
latitudeandlongitudecolumns are written with reduced precision as a privacy measure. That was only true for the API. The rounding lived inApiClient.add_run, and nothing rounded the values on the way toemissions.csvor any of the other output methods._ensure_geo_metadatacopied the raw geolocation result intoself._conf, and_prepare_emissions_dataread those values straight back out. So the CSV got whatever precision the geolocation provider returned, which for a home connection can be a lot more precise than 11 km.I moved the rounding to the one place the coordinates enter the config, so every output method now agrees.
self._geostill holds the full precision values, because the Electricity Maps carbon intensity lookup readsgeo.latitudeandgeo.longitudedirectly and would lose accuracy otherwise. I left the existinground()inapi_client.pyin place. It is now a second round that changes nothing on this path, but it still does real work for coordinates set by hand in a.codecarbon.configfile, since those never go through_ensure_geo_metadata.No documentation change is needed. The docs already describe the behaviour this makes true.
Related Issue
Fixes #1376
Motivation and Context
emissions.csvis the file people attach to papers and commit to repositories. The documentation told them the coordinates in it were already coarsened, and they were not. A documented privacy guarantee that does not hold is worse than no guarantee at all, so this takes option 1 from the issue and makes the code match what users were told.The change is visible in the output. Coordinates in new CSV files will have one decimal place instead of four or more.
How Has This Been Tested?
I added
test_coordinates_precision_is_reduced_in_emissions_datatotests/test_emissions_tracker.py. It mocks the geolocation lookup with 48.8566 / 2.3522, calls_prepare_emissions_data(), and checks the result carries 48.9 / 2.4 whiletracker._geostill holds the full precision values. I checked the test fails on the unpatched source and passes with the change.I also ran the package suite locally with
CODECARBON_ALLOW_MULTIPLE_RUNS=True. 620 passed, 3 failed. The three failures aretests/cli/test_cli_main.py::test_monitor_run_and_monitor,tests/test_docs_examples.py::test_doc_python_blocks[scikit-learn.md]andtest_task_energy_with_live_update_interference. All three fail the same way on master with my changes stashed, so they are not caused by this PR.black --checkis clean on both changed files.Screenshots (if appropriate):
Not applicable.
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:AI Usage Disclosure
Please refer to docs/how-to/ai-policy.md for detailed guidelines on how to disclose AI usage in your PR. Accurately completing this section is mandatory.
I used an AI coding tool to write the patch and this description, and I reviewed and ran everything myself before opening the PR.
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.