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

This PR decreases the minimum cardinality for Pooling.has_input to 1 #1095

Closed
wants to merge 1 commit into from

Conversation

turbomam
Copy link
Member

@turbomam turbomam commented Aug 30, 2023

@turbomam turbomam changed the title min car for Pooling has inuput = 1 min card for Pooling has inuput = 1 Sep 5, 2023
@turbomam
Copy link
Member Author

turbomam commented Sep 5, 2023

which do we really want?

@turbomam
Copy link
Member Author

@aclum this will be an easy issue and PR for me to close, if you feel confident about the minimum and or maximum number of inputs into a Pooling process.

@turbomam turbomam requested a review from aclum October 26, 2023 17:21
@turbomam turbomam marked this pull request as ready for review October 26, 2023 17:21
@turbomam turbomam changed the title min card for Pooling has inuput = 1 This PR decreases the minimum cardinality for Pooling.has_input to 1 Oct 26, 2023
@aclum
Copy link
Contributor

aclum commented Oct 27, 2023

@turbomam Where is the example data driving this change? Conceptually sample does not go through pooling for a sequencing lab workflow unless there are at least two samples so I think we should leave the minimum cardinality as 2.

@turbomam
Copy link
Member Author

@aclum this PR was opened at the end of August. I think we tried setting a minimum cardinality of three on Pooling.has_input and then found that there were some Neon Poolings with only two inputs, so we just completely removed the minimum cardinality constraint. We can leave it off, or change the minimum cardinality to 1 or 2.

But I don't have any motivating data and would be happy to close this PR without merging if you think that's best.

@aclum
Copy link
Contributor

aclum commented Oct 31, 2023

@turbomam Am i reading the schema incorrectly? I read this as the minimum cardinality for slot has_input is currently enforced as 2 in the main branch of the nmdc schema for class Pooling.

@turbomam
Copy link
Member Author

turbomam commented Nov 1, 2023

Sorry for dragging you into this, @aclum. Yes, I see what you mean and suggest that we just close this PR without merging.

@aclum
Copy link
Contributor

aclum commented Nov 1, 2023

@turbomam agree

@turbomam turbomam closed this Nov 1, 2023
@turbomam turbomam deleted the issue-1014-no-pool-min-inputs branch November 1, 2023 22:31
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