Open ebolyen opened 1 year ago
Yeah the longitudinal dependency can definitely be dropped... I think that is historical.
dropping q2-feature-classifier would require dropping a couple actions in RESCRIPt — is that what you are proposing? Or add instructions that users who wish to run those actions will need to install q2-feature-classifier separately?
No, I would want to keep those actions, but I think we can just duplicate the parameter descriptions (and move the type over to q2-types) (from these imports here: https://github.com/bokulich-lab/RESCRIPt/blob/11aedaa7dc46a3e4f0b9523b9ff4043f60e5a999/rescript/plugin_setup.py#L40-L42).
Is there a pipeline that uses feature-classifier directly that I am missing?
Is there a pipeline that uses feature-classifier directly that I am missing?
Yes, see e.g.: https://github.com/bokulich-lab/RESCRIPt/blob/11aedaa7dc46a3e4f0b9523b9ff4043f60e5a999/rescript/cross_validate.py#L38-L39
I would be very tempted to deprecated these actions though. Maybe just port those pipelines over into the classifier training workflows to use them for our purposes. I was never very happy with those pipelines.
This seems like a good candidate to finish up in this upcoming release cycle since we are adding RESCRIPt to the amplicon distro. @nbokulich I can bring this up in this week's eng meeting - your preference would still be to deprecate these actions?
Hi @lizgehret thanks for the reminder! Yes let's discuss this week.
Which dependencies are priority to drop? It looks like q2-longitudinal is indeed used in a couple places, e.g., here: https://github.com/bokulich-lab/RESCRIPt/blob/a19d106d741c3e064f6d17f2b91bf131fd28903f/rescript/evaluate.py#L51
though it is just to get the line plots from the volatility
plot. So I would favor waiting until we have similar functionality up and running in q2-visard before dropping this.
The actions depending on q2-feature-classifier could always be ported over to another plugin (a new one) to simplify RESCRIPt's dependencies and focus more on the core functionality. But as q2-feature-classifier is already in the amplicon distro anyway, is this needed?
q2-longitudinal is dropped as a dependency with #204
so we are halfway there. Should we discuss if/how to address the q2-feature-classifier dependency? This will require dropping or porting a couple actions. Porting to a new plugin could be an option, I think the classifier eval actions are probably not needed by most users.
q2-longitudinal is dropped as a dependency with #204
so we are halfway there. Should we discuss if/how to address the q2-feature-classifier dependency? This will require dropping or porting a couple actions. Porting to a new plugin could be an option, I think the classifier eval actions are probably not needed by most users.
Yeah! Maybe this would be a good thing to discuss in next month's meeting post-release? Once we have all of the grant objectives wrapped up, it'll be a good time to figure out what we can add to our plates for the next development cycle.
It looks like we may be able to drop some rather significant weight from the recipe:
I can't find an import of longitudinal, which is bringing along sample-classifier as well. And for feature-classifier, the only imports are these which I don't think is actually worth the cost of pulling feature-classifier along (requiring quality-control as well).