microbiome / mia

Microbiome analysis
https://microbiome.github.io/mia/
Artistic License 2.0
45 stars 25 forks source link

changed diversity input check #520

Closed Insaynoah closed 2 months ago

Insaynoah commented 2 months ago

I changed around the input check for the index as suggested in #519 . Sorry for the headache caused by #514.

TuomasBorman commented 2 months ago

No problem at all, it is maintainers job to evaluate the code, and I don't assume that you know yet all the things in mia. The main goal is to develop good things with the best possible quality.

  1. Can you modify also TreeSE method (same thing there, but now with additional faith index)
  2. Run BiocCheck::BiocCheck() (Check that the code added by you passes Bioconductor recommendations. There are warnings related to other lines, skip those.)
  3. Run manual tests that the code does what it should do. (Can you run the code from the issue? Can you print the code and output here?)
Insaynoah commented 2 months ago

Thanks for the tips !

I ran the BiocCheck with no warnings for the lines I modified and I ran some manual tests on top of the already existing tests, here are the results :


tse <- estimateDiversity(tse, 
                              assay.type = "counts",
                              index = "test", 
                              name = "shannon")

Output :

Error: 'index' contains unsupported indices.
tse <- estimateDiversity(tse, 
                         assay.type = "counts",
                         name = "shannon")

Output :

Error: 'name' must be a non-empty character value and have the same length as 'index'.

tse <- estimateDiversity(tse, assay.type = "counts"
                         , index = NULL,
                         name = "test")

Output :

Error in .local(x, assay.type, assay_name, index, name, ...) : 
  No 'index' specified.

Would you rather have it so that it throws an index error or a name error ?

TuomasBorman commented 2 months ago

Looks good! Run BiocCheck::BiocCheck() and fix everything related to these added lines. (There are some other things related to other lines, but let's keep them unchanged at this point)

Insaynoah commented 2 months ago

That should do it, we shouldn't use collapse within a paste call and if we do past it shouldn't be within a stop call. Therefore, I assembled the string before using it in the stop function.

Insaynoah commented 2 months ago

It should be good now, bioconductor doesn't allow a paste statement within a stop so i created a new variable that pastes the allowed indices before passing it to the stop function.