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

updated code with changes for handling 1 row per sample per cycle and 1 row per sample samplesheets #23

Closed
wants to merge 121 commits into from

Conversation

RobJY
Copy link
Contributor

@RobJY RobJY commented Mar 18, 2024

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/mcmicro branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

new format is sample,cycle_number,channel_count,image_tiles
Copy link

@josenimo josenimo left a comment

Choose a reason for hiding this comment

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

I looked through the changes.
I like the flexibility of the sample sheet, it is a bit complex at first, but it then makes sense.

I think that the testing is a bit messy regarding which are the inputs and the outputs, and what is being tested. And yeah the workflow sections have to be filled out and tests run.

@jmuhlich
Copy link
Member

I think that the testing is a bit messy regarding which are the inputs and the outputs, and what is being tested. And yeah the workflow sections have to be filled out and tests run.

The tests can probably be refactored now that the workflow inputs are more logical with the new template layout. I will have a better idea about that by the end of the week when I finish that refactor/merge.

jmuhlich and others added 16 commits March 27, 2024 14:37
* Remove defaults in nf-test.config and mark schema for staging
* Remove extra copy of schema since we are now staging the main instance
* Change nf-test local nextflow.config so it only contains overrides
* Remove more unused/duplicate nextflow config files
* Update test inputs to match new pipeline inputs
* Generate snapshot for versions output
Resources should be specifed via label in the module itself
* Add CsvUtils helper class, currently just for rounding float columns.
* Round float columns in quantification output csv content to avoid false
  snapshot mismatch due to precision issues in lowest bits.
* Ran nf-core modules update deepcell/mesmer
* Deleted local mesmer patch as it's no longer necessary
* Adjusted modules config to eliminate containerOptions (not needed any
  longer) and manually set the prefix to match our previous locally patched
  behavior
* Added test config to mount the local /tmp/.keras in the container at the same
  path to provide caching of the ~100 MB model file the container downloads at
  runtime
* Updated test snapshot for new version number
@josenimo
Copy link

josenimo commented Apr 5, 2024

Amazing work @jmuhlich,
LGTM, nice and clean.

@jmuhlich
Copy link
Member

Sorry to pull the rug out from folks who have reviewed, but after I started making substantial updates to this branch it got rather out of date with Rob's original version and we decided it would be best to create a new PR. So I'm closing this in favor of #29.

@jmuhlich jmuhlich closed this Apr 23, 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.

4 participants