Some fixes before the upcoming release #257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…obs_name_col optional in report.rmd
WackerO
changed the title
|
Is it necessary to "unbump" the versions? |
|
…ybe it works online?
…nd messed them up
pinin4fjords
requested changes
Apr 2, 2024
nextflow_schema.json
Outdated
| @@ -268,8 +268,9 @@ | |||
| "properties": { | |||
| "proteus_measurecol_prefix": { | |||
| "type": "string", | |||
| "default": "LFQ intensity ", | |||
| "description": "Prefix of the column names of the MaxQuant proteingroups table in which the intensity values are saved; the prefix has to be followed by the sample names that are also found in the samplesheet. Default: 'LFQ intensity '; take care to also consider trailing whitespace between prefix and samplenames." | |||
| "default": "LFQ intensity_s", | |||
There was a problem hiding this comment.
This seems weird and brittle- too hacky for me. People are going to misunderstand this and get it wrong. We need a different solution - why not just make the proteus module allow for an optional space after the prefix?
There was a problem hiding this comment.
I generally prefer not having my code assume that a whitespace is necessary and instead just take directly what the user inputs...but I suppose here, the flexible whitespace addition is the best solution. I just updated the proteus module with those changes
Co-authored-by: Jonathan Manning <jonathan.manning@seqera.io>
Co-authored-by: Jonathan Manning <jonathan.manning@seqera.io>
Co-authored-by: Jonathan Manning <jonathan.manning@seqera.io>
…ntialabundance into prerelease_fixes
…o python); undid the changes to the docs/defaults for --proteus_measurecol_prefix regarding the final space
…into prerelease_fixes
pinin4fjords
requested changes
Apr 22, 2024
modules/local/filter_difftable.nf
Outdated
| @@ -2,10 +2,10 @@ process FILTER_DIFFTABLE { | |||
|
|
|||
| label 'process_single' | |||
|
|
|||
| conda "conda-forge::gawk=5.1.0" | |||
There was a problem hiding this comment.
Re-writing this process in Python is a quite a major change to smuggle into a PR of small fixes ;-).
Can you say why the process was re-written, and demonstrate that the output is the same relative to previous?
There was a problem hiding this comment.
If you prefer, I can also do the module change in a separate PR, that's not a problem!
Nope, can't demonstrate that, the output changed; it was incorrect before, that's why I had to change it :') AWK suddenly filtered out all the genes and just produced an empty table (was not the case when I originally added the module).
I tried a lot to change it in AWK but it simply did not work and I have to be honest, I found the AWK code quite painful to deal with as I'm simply not experienced enough with it.
There was a problem hiding this comment.
Oh, but as a follow-up, I did check the output. The new module + this code makes it so that the results are the same as the DE genes according to the html report, see here:
There was a problem hiding this comment.
Actually, could you raise this as a separate PR? I think that would make more sense and you can post the reasoning and the evidence there.
There was a problem hiding this comment.
Yes of course, see #264
10 tasks
…into prerelease_fixes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
added maxquant profile to nextflow.config, clarified some docu, made obs_name_col optional in report.rmd,
fixed FILTER_DIFFTABLE module (changed to python as AWK stopped filtering correctly)(done in #264)PR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).