Skip to content

EthiopianDateParser: use the computed Gregorian month for the year-offset branch - #849

Closed
ManasBagul23 wants to merge 1 commit into
dbpedia:masterfrom
ManasBagul23:fix-ethiopian-date-gregorian-year
Closed

ManasBagul23 wants to merge 1 commit into
dbpedia:masterfrom
ManasBagul23:fix-ethiopian-date-gregorian-year

Conversation

@ManasBagul23

@ManasBagul23 ManasBagul23 commented Sep 25, 2026 •

Copy link
Copy Markdown

Related to #845 (item A only).

geezToGregorianDateConverter implements the standard Julian-Day-Number-to-Gregorian conversion (Fliegel & Van Flandern). That algorithm's year-offset step branches on the computed Gregorian month (year = C-4715 when the Gregorian month is January/February, else C-4716) — it has nothing to do with which calendar the input date was originally in.

The code branched on month, the Ethiopian input month, instead of the gregorianMonth value it had already computed two lines above:

val gregorianMonth: Long = if (E - 1 <= 12) E - 1 else E - 13
val gregorianYear: Long = if (month <= 2) C - 4715 else C - 4716  // bug: should branch on gregorianMonth

Ethiopian months 1–2 (Meskerem/Tikimt, ~September/October) and 5–6 (Tir/Yekatit, ~January/February) straddle the Gregorian year boundary in the opposite sense from their own month numbers, so exactly those months hit the wrong branch — matching #845's report that only months 1, 2, 5, 6 are affected:

  • Ethiopian month 1 or 2 (≤2, old code used C-4715) actually falls in Gregorian September/October (>2, needs C-4716) → year off by +1.
  • Ethiopian month 5 or 6 (>2, old code used C-4716) actually falls in Gregorian January/February (≤2, needs C-4715) → year off by −1.

Verification: reimplemented the algorithm standalone in Python and ran it against every example in #845's evidence table (the article's own Gregorian gloss for each Ethiopian date). All nine reproduce the reported wrong year with the old branch condition and the correct year with gregorianMonth — e.g. Meskerem 7, 1984 (Ethiopian) gives old=1992, new=1991, matching ሊኑክስ's own gloss of 17 Sep 1991. No Scala/sbt toolchain available locally to run the project's own test suite.

This fixes only item A of #845 (the year-offset bug). Item B (the ቀን date-format regex never matching) is a separate, unrelated fix and is left for a follow-up.

Prepared with AI assistance; I reviewed the changes.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected Gregorian year calculation when converting Ethiopian dates that fall in January or February.

…ian input month, for the year-offset branch

geezToGregorianDateConverter implements the standard Julian-Day-Number-to-
Gregorian conversion (Fliegel & Van Flandern). That algorithm's year-offset
step branches on the *computed Gregorian* month (year = C-4715 when the
Gregorian month is January/February, else C-4716) -- it has nothing to do
with which calendar the input date was in.

The code branched on `month`, the Ethiopian input month, instead of the
`gregorianMonth` value it had already computed two lines above. Ethiopian
months 1-2 (Meskerem/Tikimt, ~September/October) and 5-6 (Tir/Yekatit,
~January/February) straddle the Gregorian year boundary in the opposite
sense from their own month numbers, so exactly those months hit the wrong
branch:

- Ethiopian month 1 or 2 (<=2, so the old code used C-4715) actually falls
  in Gregorian September/October (>2, needs C-4716) -> year off by +1.
- Ethiopian month 5 or 6 (>2, so the old code used C-4716) actually falls
  in Gregorian January/February (<=2, needs C-4715) -> year off by -1.

Verified against every example in dbpedia#845 (a standalone reimplementation of
the algorithm in Python, run against the exact Ethiopian dates and their
Gregorian year given in the issue): all nine reproduce the reported wrong
year with the old branch and the correct year with `gregorianMonth`, e.g.
Meskerem 7, 1984 (Ethiopian) is old=1992, new=1991, matches the article's
own gloss of 17 Sep 1991. No Scala/sbt toolchain available locally to run
the project's own test suite.

This fixes only item A of dbpedia#845 (the year-offset bug). Item B (the `ቀን`
date-format regex never matching) is a separate, unrelated fix left for a
follow-up.

Related to dbpedia#845 (item A only)
Copilot AI lite review requested due to automatic review settings September 25, 2026 08:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: c558ec26-ede3-491a-b0b0-5ff29a90d68e

📥 Commits

Reviewing files that changed from the base of the PR and between e3dfe8b and 47f7854.

📒 Files selected for processing (1)
  • core/src/main/scala/org/dbpedia/extraction/dataparser/EthiopianDateParser.scala

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The Ethiopian date converter now selects the Gregorian year offset using the calculated Gregorian month. Gregorian months January and February use C - 4715; other months use C - 4716.

Changes

Ethiopian date conversion

Layer / File(s) Summary
Gregorian year selection
core/src/main/scala/org/dbpedia/extraction/dataparser/EthiopianDateParser.scala
The converter checks the calculated Gregorian month to select the year offset. Months 1–2 use C - 4715; other months use C - 4716.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 47f78

The change selects the Gregorian year based on the converted month, correcting dates that fall in January or February. The inspected boundary calculations support this behavior, and no concrete merge-blocking risk is established; normal project checks remain appropriate.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: using the computed Gregorian month for the year-offset branch.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

@jimkont

jimkont commented Sep 28, 2026

Copy link
Copy Markdown
Member

Thanks for the controbution @ManasBagul23

@jimkont

jimkont commented Sep 28, 2026

Copy link
Copy Markdown
Member

there was some recent merge on this parser, can you rebase your changes on the latest code and check if the fix is still needed @ManasBagul23

@ManasBagul23

Copy link
Copy Markdown
Author

Checked — PR #846 (merged as c870f5d) already fixes this exact bug the same way (branches the year offset on gregorianMonth instead of the Ethiopian input month), plus the arity mismatch from #850 and the ቀን day-marker gap from #845 item B, all in one pass. Closing this one as superseded rather than rebasing something that's no longer needed — thanks for the quick review @jimkont.

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