Thoughts on my PRs?

Downstream analysis and statistics MaAsLin
1

Hi Biobakery team,
Just wanted to see if any of the PR’s I submitted for MaAslin2 were considered, and if there is anything I could do to move things along?

2

Hi,

Generally speaking, we don’t merge external pull requests unless they are simple bug fixes or other relatively trivial changes. We find that changes made by users are sometimes too specific to their own use cases to make sense to implement for the distributed version or in cases where we do want to add suggested features, we tend to do the programming on our end to make sure they work properly with a wide variety of inputs, follow our coding style, etc.

However, of course we are always happy to field feature requests and other code related issues, and it’s great to see people engaging with the bioBakery tools on that level. In the future, it is probably best to make a forum post for each specific issue, since this is attended to more regularly than the pull requests, and we want to foster discussion here. Thanks!

3

Hi @tkuntz-hsph ,
This is pretty frustrating. I’m not expecting PR’s to be merged without a (sometimes significant) amount of back and forth and discussion with the repository maintainer; opening the PR is the start of the conversation. Sure, this could have been started with a forum post instead, but in this case I needed these changes for my work and only considered the potential wider benefit after the fact (as I mentioned in the PRs).

Regarding your response,

  1. If you don’t generally merge pull requests, you should have this clearly stated in a contributions document somewhere within the biobakery world. A potential place for that would be in the issues template you use to re-direct discussion here.
  2. Rejecting pull requests and then re-implementing the same feature without attribution is rather rude. When you reject PRs but then re-implement the feature, my public record just shows a rejected PR with no reference to how my idea was incorporated into the repo. As developers, our public contributions record is super important.
    3). I believe I made a case that the changes I introduce are not “too specific to [my] own use cases” for all of these.
  • For #7, Defaulting to filtering counts/abundances by a non-relative min_abundance fails for two cases: (a) trying to filter count data introduces biases based on sequencing depth, and (b) there is no error when trying to filter count data by a float, as nothing gets removed. I discussed this in the PR. Allowing users to pass floats silently to filter count data obviously needs to be remedied at least with a warning – I would gamble that plenty of users are doing this unwittingly (as I did for several weeks before finding that this was an issue). The manual specifies that counts or relative abundances can be provided, but the filtering examples all assume relative abundance.
  • For #9, when the manual says Unfortunately, MaAsLin2 does not provide a direct interface for this. it makes it sound like that would be a nice feature to have. Making people manually construct interaction terms in a tool for make running regressions easier seems a bit contradictory to me.
  1. Regarding to make sure they work properly with a wide variety of inputs, I made sure the code passed existing tests, and I went though the process of creating specific tests for the features I was suggesting. If there are additional tests being done internally, these should be formally added.
  2. Regarding , follow our coding style, etc., there is no obvious style guide being used. Using tools like styler results in thousands of lines needing changes to make code style compliant – I’m not going to do that on a PR cause that would make the actual feature nearly impossible to see in the diff. And again, if there was a style guide being used, that should be mentioned in a contributors document, not as a reason to reject a PR.

Let me know how you would like to proceed; I’m happy to continue discussing filtering and the interactions issues.

4

Hi @nickp60 - sorry it’s taken a while to get back to you; as per @tkuntz-hsph 's response earlier, we didn’t previously have an infrastructure for activities like this, and we really appreciate your input in getting one set up! Although it’s just not possible for us to maintain the personnel and bandwidth to handle most unsolicited PRs, we were able to put together some options we hope will facilitate urgent PRs (e.g. bug fixes) while also providing a route to coordinate on new features.

Specifically, we’ve created a PR template at:

that directs new feature requests (for any bioBakery component) to a forum newly-created for this purpose:

This allows us to coordinate code changes from our end or from external contributors more seamlessly prior to the investment of making a PR. For urgent or small bug fixes, the template allows PR submission directly.

To accommodate both types of material, we’ve also clarified the attribution policy - including you as the inspiration for this process, many thanks again!