d3b-center / annoFuse

Filter and prioritize fusion calls
Other
20 stars 3 forks source link

Add custom type annotations #97

Closed sakshamphul closed 1 year ago

sakshamphul commented 2 years ago

Purpose/implementation Section

This PR adds desired custom type annotation that checks for required columns and maps input columns to output columns using config file

What feature is being added or bug is being addressed?

Ability of fusion standardization function to accept custom type input that checks for few required columns. Config file has been made optionally and can be use to map input columns with output columns. Additionally, it can be used to generate output columns with specific header name and 'NA' as an entry (Usage: "None" :: "Output_name")

What was your approach?

Developed custom type on top of existing starfusion and arriba. Package will continue to support both of these caller types with same implementation.

What GitHub issue does your pull request address?

https://app.zenhub.com/workspaces/d3b-bfx-5cdd720eed0dce4209e0b8a5/issues/d3b-center/bixu-tracker/1533

Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.

All the unit test were successful related with fusion standardization function. A new test was also added for custom type

Which areas should receive a particularly close look?

fusion standardization function

Is there anything that you want to discuss further?

Testing this version of the package will require installing annoFuse locally. Here are the steps to install it locally: 1) Git clone this branch 2) Uninstall previous version of annoFuse remove.packages("annoFuse") 3) Install local version devtools::install("path/to/package/folder") 4) Now, you should be able to test fusion_standardization function.

Documentation Checklist

Package Checklist

federicomarini commented 2 years ago

As an alternative to Prettier, there's the styler package which already implements most of the style guidelines I (try to) follow :) Content-focused review will follow 😉

sickler-alex commented 2 years ago

I was able to test the updated code with star and arriba files and they produce the same output as previous versions which is expected.

federicomarini commented 1 year ago

Well done, the examples run correctly now!

federicomarini commented 1 year ago

One last thought before the full approval: Please consider if we might need somewhere "outside" the new main function the shape_output function. If that is the case, just proceed to document it fully via roxygen and so. Actually, even if it is not exported, it is good practice (= "would never hurt") to keep it fully documented even if not exported. This makes the whole debugging-upon-changes much easier 😉

sakshamphul commented 1 year ago

Ok, I added few comments for shape_output function and updated the documentation via roxygen

federicomarini commented 1 year ago

Seems all good to me now 😉 If you need the formal approval of someone else, just ping them. Otherwise, looks like it is ready to merge.

federicomarini commented 1 year ago

I had to restore some indentation, but otherwise to me it looks ready to merge

federicomarini commented 1 year ago

merging done via CLI 🎉

sakshamphul commented 1 year ago

Thank you @federicomarini for all the reviews, commits and merging this branch.

sakshamphul commented 1 year ago

Hi @federicomarini How can we push these changes to CRAN webserver (release latest version)? Do we need to push to CRAN webserver?

federicomarini commented 1 year ago

At the moment, this is not on CRAN or Bioc. So the installation has to go via Github, as it was before our tiny extension. The plan to bring this to Bioc is alive and running, and it was mainly the motivation to split up the package in code (this) and data (annoFuseData). Whether to proceed to Bioc would likely involve mainly @jharenza (decision-wise).

migbro commented 1 year ago

@federicomarini @sakshamphul , now that the changes have been merged into master, I strongly suggest a release be made, following the guidelines here: https://semver.org/, so probably v0.90.0 -> v0.91.0. It'd be a nice tidy way to allow users to use their preferred version easily as well as get a concise summary of what has changed since the last merge. Please let me know if you have any questions or comments. Thanks!

federicomarini commented 1 year ago

Good point, also since we have the news.md file to keep track of this. At some point we would (have to) jump to 0.99.0 if we enter the process of the Bioc submission, happy to oversee that if we want to go that way. @sakshamphul, do you want to take over in a separate branch to update the NEWS and bump up the version number?

sakshamphul commented 1 year ago

Sure! Just to confirm, latest version would be 0.91.0 ?

federicomarini commented 1 year ago

Yeah, we are fairly free on this but that would be a reasonable choice. The x.y.Z number would be what goes up with tiny devel steps (e.g. in one release cycle of 6 months as on Bioc). So, yes, go with 0.91.0