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

agglomeration functions and na.rm #658

Open
wants to merge 9 commits into
base: devel
Choose a base branch
from
Open

agglomeration functions and na.rm #658

wants to merge 9 commits into from

Conversation

TuomasBorman
Copy link
Contributor

In this issue #654, we noticed that the use of na.rm is not intuitive in agglomeration functions.

This PR adds option for excluding NAs in agglomeration functions (agglomerateByRank, agglomerateByVariable and agglomerateByPrevalence). na.rm now works similarly to sum(x, na.rm = TRUE) in these aforementioned functions.

In agglomerateByVariable na.rm controlled whether we remove those rows that did not have any group information, i.e., their group was NA. Now this parameter is named group.rm.

In agglomerateByRank na.rm controlled whether we remove those rows that did not have any taxonomy information. Now this parameter is called empty.rows.rm.

These parameter names can be still modified. Maybe we could unify so that both would have parameter empty.row.rm.

na.rm is now handled in mia, but I opened issue in scuttle if this can be handled directly in function that mia utilizes: LTLA/scuttle#33

Let's wait until the issue in scuttle is solved.

@antagomir
Copy link
Member

How about empty.group.rm or (group.na.rm and row.na.rm) to enhance consistency?

@TuomasBorman
Copy link
Contributor Author

row.rm might also be one option to keep it short. I think we could use same name for both

@antagomir
Copy link
Member

ok to me as long as it is consistent.

@TuomasBorman TuomasBorman changed the title agglomeraton functions and na.rm agglomeration functions and na.rm Nov 12, 2024
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