Skip to content

Add nan parameter to fcoalesce for NaN/NA distinction control#7189

Merged
MichaelChirico merged 8 commits intomasterfrom
issue_4567
Jul 21, 2025
Merged

Add nan parameter to fcoalesce for NaN/NA distinction control#7189
MichaelChirico merged 8 commits intomasterfrom
issue_4567

Conversation

@Mukulyadav2004
Copy link
Copy Markdown
Contributor

fixes #4567

Currently fcoalesce always treats NaN as missing (same as NA_real_), providing no user control over NaN handling. This inconsistency with nafill, which offers a nan parameter for controlling whether NaN should be treated distinctly from NA.

This PR adds a nan parameter to fcoalesce with default behavior nan=NA (treating NaN as missing, default) and option nan=NaN (preserving NaN values), ensuring fcoalesce(x, y, nan=Z) behaves consistently with nafill(x, fill=y, nan=Z) when y is length-1.

Implemetation:
In wraper.R - Added nan parameter with default behaviour - nan = NA means treat NaN as NA - in both fcoalesce as well setcoalesce
Then update header - data.table.h to match new function signature.

In coalesce.c

  • Add third parameter - nan_is_na_arg to support user control
  • After then extract and validate the nan control parameter
  • Apply user's nan preference when checking scalar fill values, for the final value checking and in main replacement loop.

The logic is :
nan_is_na(NA) -> TRUE - then use ISNAN(val) - thus both NaN and NA_real treated as missing
nan_is_na(NaN) -> FALSE - then use ISNA(val) - thus only NA_real treated as missing, NaN preserved

In Docs change - coalesce.Rd :

  • Added nan parameter to both function signatures
  • Added explanation of how nan = NA vs nan = NaN affects behaviour.
  • Demonstrate the difference between default and explicit nan = NaN behaviour

@MichaelChirico @jangorecki can you please review

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.77%. Comparing base (2b191ae) to head (2ce965f).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7189   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files          81       81           
  Lines       15204    15215   +11     
=======================================
+ Hits        15018    15029   +11     
  Misses        186      186           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 17, 2025

  • HEAD=issue_4567 slower P<0.001 for DT[by,verbose=TRUE] improved in #6296
    Comparison Plot

Generated via commit 2ce965f

Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 3 minutes and 3 seconds
Installing different package versions 41 seconds
Running and plotting the test cases 2 minutes and 38 seconds

Comment thread man/coalesce.Rd Outdated
}
\usage{
fcoalesce(\dots)
fcoalesce(\dots, nan = NA)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
fcoalesce(\dots, nan = NA)
fcoalesce(\dots, nan=NA)

Comment thread src/coalesce.c Outdated
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

LGTM! Please add a NEWS entry

@MichaelChirico
Copy link
Copy Markdown
Member

sorry, last step is to merge master to resolve the conflict

@Mukulyadav2004
Copy link
Copy Markdown
Contributor Author

Done, sorry for the delay, my laptop's been quite slow but the merge is complete now.

@MichaelChirico MichaelChirico merged commit 66cb6d2 into master Jul 21, 2025
12 checks passed
@MichaelChirico MichaelChirico deleted the issue_4567 branch July 21, 2025 19:19
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.

fcoalesce will not replace NaN with NA_real_

2 participants