search

qiime2 / q2-diversity

BSD 3-Clause "New" or "Revised" License
4 stars 45 forks source link

imp: adonis - tighten scope of nan-checking; tests #303

Closed thermokarst closed 3 years ago

thermokarst commented 3 years ago

This is a follow-up PR for https://github.com/qiime2/q2-diversity/pull/282

cc @collin-le & @nbokulich

This PR tightens up the NaN checking to only the columns used in the formula, rather than raising an error on any column, even if it isn't used.

This changeset also adds a few quick unit tests to exercise this behavior.

thermokarst commented 3 years ago

add an option to drop NaNs instead of failing?

I'm game - but how about we do it in a future dev cycle? I just wanted to wrap up @collin-le's work on this in time for the upcoming release next week.

Some tests are failing locally but this does not appear related to this PR.

Can you share some logs? I'm all green here.

nbokulich commented 3 years ago

but how about we do it in a future dev cycle?

👍 opening an issue momentarily...

Can you share some logs? I'm all green here.

I can share out of loop but the error is clearly unrelated, coming from core-metrics. I am going to go ahead and hit MERGE 🟢

thermokarst commented 3 years ago

I can share out of loop but the error is clearly unrelated, coming from core-metrics

There was a huge shuffle this summer related to q2-div-lib - you might need to refresh your dev env, in case you haven't in a while.