Skip to content

Add numtest and use doctest.ELLIPSIS_MARKER to fix doctest errors when doing floating point number comparisons#408

Merged
ielis merged 3 commits intodevelopfrom
fix_fp_error_in_doctests
Jan 22, 2025
Merged

Add numtest and use doctest.ELLIPSIS_MARKER to fix doctest errors when doing floating point number comparisons#408
ielis merged 3 commits intodevelopfrom
fix_fp_error_in_doctests

Conversation

@justaddcoffee
Copy link

doctests in survival.rst and phenotype_scores.rst are failing for me on my Macbook M1 MacOS v12.5

FAILED               [  2%]
docs/user-guide/analyses/survival.rst:0 ([doctest] survival.rst)
120 
121 We execute the analysis by running
122 
123 >>> result = survival_analysis.compare_genotype_vs_survival(
124 ...     cohort=cohort,
125 ...     gt_clf=gt_clf,
126 ...     endpoint=endpoint,
127 ... )
128 
129 >>> result.pval
Expected:
    0.06200425830044376
Got:
    0.06200425830044377

/Users/jtr4v/PythonProject/gpsea/docs/user-guide/analyses/survival.rst:129: DocTestFailure

This PR adds numtest as a test dependency and then uses ELLIPSIS_MARKER to allow approximate comparisons

@justaddcoffee justaddcoffee requested a review from ielis January 21, 2025 20:08
Copy link
Collaborator

@ielis ielis left a comment

Choose a reason for hiding this comment

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

Hello @justaddcoffee,

thanks a lot, using ellipsis in float comparisons is a very good and useful idea. I have a few comments & questions, can you pls see below?

Thank you!

>>> p_value = r.pvalue
>>> float(p_value)
6.348081479150902e-06
>>> float(p_value) # doctest: +ELLIPSIS
Copy link
Collaborator

Choose a reason for hiding this comment

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

The doctest runner seems to recognize the ... even without #doctest: +ELLIPSIS marker, and the test passes here. Does the same happen on your end? If yes, pls remove # doctest: +ELLIPSIS.

pyproject.toml Outdated
test = [
"pytest>=7.0.0,<8.0.0",
"pytest-cov",
"numtest"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure we really need this dependency. All tests passed on my end without numtest, just with the ... at proper places. Unless I am missing something, we do not need numtest, and by applying Occam's razor, it should be removed.

…IS_MARKER = '...' and DOCTEST +ELLIPSIS annotations
@justaddcoffee
Copy link
Author

Thanks @ielis - you are right, the numtest dependency, doctest.ELLIPSIS_MARKER = '...' and DOCTEST +ELLIPSIS annotations are not necessary here, so I've removed them. It looks like all this ELLIPSIS stuff is now default, and I was reading some stale documentation

@ielis ielis self-requested a review January 22, 2025 15:40
Copy link
Collaborator

@ielis ielis left a comment

Choose a reason for hiding this comment

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

Looks great now, thank you!

@ielis ielis merged commit 8d2fdc8 into develop Jan 22, 2025
3 checks passed
@ielis ielis deleted the fix_fp_error_in_doctests branch January 22, 2025 15:43
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.

2 participants