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

adding min_partitions to import_bed #5932

Merged
merged 7 commits into from
Apr 24, 2019

Conversation

konradjk
Copy link
Collaborator

No description provided.

@tpoterba
Copy link
Contributor

should we just accept **kwargs and pass them to import_table?

@jigold
Copy link
Collaborator

jigold commented Apr 22, 2019

Won't the docs for that be confusing?

@tpoterba
Copy link
Contributor

I dunno. That does seem like the easiest way to make sure everything stays consistent

@konradjk
Copy link
Collaborator Author

How would type-checking and docs work for that?

@tpoterba
Copy link
Contributor

docs would say something like "all optional arguments to import_table are valid arguments to import_bed". Typechecking would happen on the import_table call

@konradjk
Copy link
Collaborator Author

Looks like it needs some sort of typecheck though and i'm not sure what to do with it:

RuntimeError: import_bed: invalid typecheck signature: function parameters with no defined type: ['kwargs']

@konradjk
Copy link
Collaborator Author

How's this @tpoterba ?

jigold
jigold previously requested changes Apr 24, 2019
@@ -656,7 +659,8 @@ def import_bed(path, reference_genome='default', skip_invalid_intervals=False) -
'f2': tint32, 'f3': tstr,
'f4': tstr},
comment=["""^browser.*""", """^track.*""",
r"""^\w+=("[\w\d ]+"|\d+).*"""])
r"""^\w+=("[\w\d ]+"|\d+).*"""],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good, but my one concern is if someone overrides one of these options that we know are correct for bed files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean? If someone passes reference_genome='GRCh38' it will correctly go to import_bed, but anything not in the signature will go on to import_table - if down the line, someone adds reference_genome to import_table, that will be a problem, but not sure if that is fixable here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If someone overrides these they'll get an error that you tried to pass the same argument twice. Maybe better to explicitly check for these options, but it seems hard to really screw yourself here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ohhhh you mean like comment=? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe document which ones are used or something?

@konradjk konradjk requested a review from jigold April 24, 2019 14:41
@konradjk konradjk dismissed jigold’s stale review April 24, 2019 14:41

documented used arguments

@danking danking merged commit 7c94df9 into hail-is:master Apr 24, 2019
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.

None yet

4 participants