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

Fix to allow ancestor_too_old() tests to be done #1776

Closed
wants to merge 1 commit into from

Conversation

CameronD73
Copy link
Contributor

@CameronD73 CameronD73 commented Sep 13, 2024

When calling probably_alive, the previous code did not clear the self.pset list between running descendant test and ancestor test, so the ancestor test code assumed all persons had already been checked and was never executed.

This bug and fix is easier to evaluate if patch for bug #13431 has been applied - that is just to improve debug output.

@Nick-Hall Nick-Hall added the bug label Sep 13, 2024
@CameronD73
Copy link
Contributor Author

The failures in the unit tests are probably expected as a result of the bug fix

  • The geneweb export test added a zero death date to indicate known death but unknown date because the bug fix now does the ancestor test, which returns "probably dead by 2019" for I0042, since his parents were born 1889. However his only record is being adopted (with no date), so the assumptions behind the calculations are to some extent not applicable
  • The "probably alive in 1900" test returned quite a few differences
    -- 4 who should have been tagged as alive but were not in the released version 5.2.3
    -- 45 who were tagged as probably alive in 1900 in released ver 5.2.3 but were very unlikely alive. Some were definitely wrong in 5.2.3, some might be alive by a long stretch, but had very little associated detail, One was incorrect in the proposed bug fix.

@Nick-Hall Nick-Hall changed the title Fixes Bug #0013433 - allow ancestor_too_old() tests to be done Fix to allow ancestor_too_old() tests to be done Dec 19, 2024
@giotodibondone
Copy link

giotodibondone commented Dec 19, 2024

When calling probably_alive, the previous code did not clear the self.pset lis

fyi probably_alive code being updated by
" proposed code changes for issue#0013443 in ProbablyAlive code #1790 "

@Nick-Hall
Copy link
Member

Enabling the unit tests that were skipped by mistake is obviously a good idea. However, I cannot merge this with failing unit tests.

@CameronD73
Copy link
Contributor Author

Enabling the unit tests that were skipped by mistake is obviously a good idea. However, I cannot merge this with failing unit tests.

The unit test result differences are effectively counting the number of errors that this bug has caused. The unit test result is simply a number that enforces bug-compatibility with current code. So, should I be incorporating the modified unit test result numbers in the same PR? (you can tell I've not done this sort of stuff before).

However, it is probably not worth the effort to document "why" in more detail, when this change is also in the more substantial reworking in #1790 where errors in the unit tests are analysed in more detail. That code change is completed, as far as I am concerned.

Previous code did not clear the self.pset list between running descendant
test and ancestor test, so ancestor test code assumed all persons had
already been checked.

Fixes #13433.
@Nick-Hall Nick-Hall changed the base branch from maintenance/gramps52 to master December 20, 2024 21:53
@Nick-Hall
Copy link
Member

OK. I have rebased this onto the master branch. We can review it alongside PR #1790.

@CameronD73
Copy link
Contributor Author

OK. I have rebased this onto the master branch. We can review it alongside PR #1790.

It is already part of PR #1790 in any case, so it does not need to be a separate PR on the master. It might be simpler just to ignore it/reject it

@Nick-Hall
Copy link
Member

Closing this as it is part of #1790.

@Nick-Hall Nick-Hall closed this Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants