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

Add support for dtype changes and better chunk calc #49

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hardingnj
Copy link
Contributor

  • includes dtype in calculation for chunk size
  • allows casting to other dtype
  • allows fill na

@alimanfoo I'm a bit worried about the fill_na step. I'm not sure if there is a better way to do this. Line 183 below.

Snakemake + pipeline changes to follow.

- includes dtype in calculation for chunk size
- allows casting to other dtype
- allows fill na
@hardingnj hardingnj requested review from a team and removed request for alimanfoo October 30, 2019 12:09
@hardingnj
Copy link
Contributor Author

Have changed the fill_na step. Tests seem to run ok, and am currently applying to all 29 samplesets on the cluster.

Review welcome- would appreciate input from @amakunin with his experience of snakemake

@hardingnj
Copy link
Contributor Author

I'm hitting an issue in zarr where if I have multiple jobs creating similar groups, e.g 2L/calldata/MQ and 2L/calldata/AD, even when require_group is used.

The error message suggests that 2L/calldata already exists. I guess this is because there is some race condition if they get created almost simultaneously? Not sure if there is a good way to avoid this? It happens very rarely, so I don't think it's breaking. But would be nice to work around if possible.

Copy link
Collaborator

@amakunin amakunin left a comment

Choose a reason for hiding this comment

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

Sorry @hardingnj, just saw this one. Do you still need my review on the issue? From the brief look I cannot see any problems with the code

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