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

ENH: accept additional featuretable types #231

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Conversation

nbokulich
Copy link
Member

fixes #230

Adds additional feature table types to the acceptable inputs for various classification and regression methods. For now I have only added types that have non-negative values or at least have descriptions to clarify what these contain (i.e., not balance or percentile abundance).

Question: would you like to see unit tests for each type? I checked out q2-feature-table for precedent, where it looks like different types are not tested for actions that accept multiple types. As the internal format is identical for these types, any tests would be redundant. So for now I have not added tests so we can discuss first.

@gregcaporaso
Copy link
Member

gregcaporaso commented Dec 18, 2023

If the view type isn't changing, which it isn't, I don't think we need additional tests here presuming that you've tested it locally with the different types.

@ebolyen, what do you think?

@gregcaporaso gregcaporaso self-assigned this Dec 18, 2023
@ebolyen
Copy link
Member

ebolyen commented Dec 18, 2023

I would agree, we don't need tests so long as the behavior doesn't rely on the structure of the data being different (which could vary depending on the semantic type). Since this is clearly just an operation on the view-type itself and not on the data per-se, there's no reason to add more tests.

@ebolyen ebolyen merged commit 7aef7e7 into qiime2:dev Dec 18, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

ENH: classify-* and regress-* actions should accept FeatureTable[RelativeFrequency]
3 participants