Make time of moon measurement clearer - #5
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces user-configurable measurement times and date ranges to the lunar data calculation script, updating the README documentation accordingly. Feedback on the changes suggests avoiding the introduction of pandas as a heavy dependency by reverting to Python's built-in csv and datetime modules. Additionally, the reviewer noted that the download link in the README needs to be updated to match the new default filename, and pointed out a syntax error in the configuration examples within both the README and the script comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import ephem | ||
| import datetime | ||
| import csv | ||
| import pandas as pd |
There was a problem hiding this comment.
Introducing pandas as a dependency solely for generating a date range and writing a CSV file adds significant overhead and a heavy external dependency. The script can easily achieve the same functionality using Python's built-in csv module, which was used in the previous version.
| import pandas as pd | |
| import csv |
| rows = [] | ||
| current_decade = None | ||
| for date in pd.date_range(start=start_date, end=end_date, freq="D"): | ||
| measurement_datetime = datetime.datetime.combine(date.date(), measurement_time_utc) | ||
| decade = (measurement_datetime.year // 10) * 10 | ||
| if decade != current_decade: | ||
| current_decade = decade | ||
| print(f"Processing decade {decade}s") | ||
|
|
||
| ephem_date = ephem.Date(measurement_datetime) | ||
| prev_new = ephem.previous_new_moon(measurement_datetime) | ||
| next_new = ephem.next_new_moon(measurement_datetime) | ||
| moon_phase = (ephem_date - prev_new) / (next_new - prev_new) | ||
|
|
||
| rows.append({ | ||
| 'Date': date.strftime('%Y-%m-%d'), # ISO 8601 | ||
| 'Time_UTC': measurement_datetime.strftime('%H:%M:%S'), # 24-hour format | ||
| 'Area': round(illuminated_area(moon_phase), 4), | ||
| 'Category': calculate_moon_phase_category(moon_phase), | ||
| 'Phase': round(moon_phase, 4), | ||
| }) | ||
|
|
||
| df = pd.DataFrame(rows, columns=['Date', 'Time_UTC', 'Area', 'Category', 'Phase']) | ||
| df.to_csv(filename, index=False) | ||
| print( | ||
| f"Run completed successfully for {filename}: " | ||
| f"start={start_datetime:%Y-%m-%d %H:%M:%S} UTC, " | ||
| f"end={end_datetime:%Y-%m-%d %H:%M:%S} UTC, " | ||
| f"rows={len(df)}" | ||
| ) |
There was a problem hiding this comment.
Instead of using pandas to generate the date range and write the CSV, we can use Python's built-in datetime and csv modules. This avoids loading all rows into memory at once and removes the heavy pandas dependency entirely.
current_decade = None
total_rows = 0
with open(filename, 'w', newline='') as csvfile:
fieldnames = ['Date', 'Time_UTC', 'Area', 'Category', 'Phase']
writer = csv.DictWriter(csvfile, fieldnames=fieldnames)
writer.writeheader()
for n in range((end_date - start_date).days + 1):
date = start_date + datetime.timedelta(days=n)
measurement_datetime = datetime.datetime.combine(date, measurement_time_utc)
decade = (measurement_datetime.year // 10) * 10
if decade != current_decade:
current_decade = decade
print(f"Processing decade {decade}s")
ephem_date = ephem.Date(measurement_datetime)
prev_new = ephem.previous_new_moon(measurement_datetime)
next_new = ephem.next_new_moon(measurement_datetime)
moon_phase = (ephem_date - prev_new) / (next_new - prev_new)
writer.writerow({
'Date': date.strftime('%Y-%m-%d'),
'Time_UTC': measurement_datetime.strftime('%H:%M:%S'),
'Area': round(illuminated_area(moon_phase), 4),
'Category': calculate_moon_phase_category(moon_phase),
'Phase': round(moon_phase, 4),
})
total_rows += 1
print(
f"Run completed successfully for {filename}: "
f"start={start_datetime:%Y-%m-%d %H:%M:%S} UTC, "
f"end={end_datetime:%Y-%m-%d %H:%M:%S} UTC, "
f"rows={total_rows}"
)|
@isaacbernat please let me know if you're put off by the pandas approach. I was hoping to get a speed boost by getting all the data from ephem, then writing as a single dataframe to CSV. It turns out though that you have to call ephem for one date at a time, so the time savings are limited only to the writing stage. If you prefer to eliminate pandas as a dependency, to keep this repo as lightweight as possible, no problem from my perspective. |
Issue: the original time of measurement was implicitly 00:00:00 (zero hundred hours, or midnight), but this was not explicitly stated in the readme,
moon_data_ephem.py script, or outputs.Proposed resolutions in this pull request:
Known tradeoffs:
Checks: I've included an Excel check zero file to compare the previous output to the new output, which is identical except for the insertion of a new time column.