Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Add parametric.py module, add plot_regresion_profiles to plot.py, and update plot_als_comparison.py example #148

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

earoy
Copy link
Collaborator

@earoy earoy commented Jun 11, 2024

Added a new module parametric.py that performs OLS and LME modeling over the nodes of a given tract to allow for group comparisons, while controlling for potential covariates. Additionally, added another plotting function to plot.py that allows the user to visualize the "tract profiles" generated by the linear models and updated the plot_als_comparison.py example to demonstrate how to use these functions.

@earoy earoy changed the title Add parametric.py module, add plot_regresion_profiles to plot.py, and update plot_als_comparison.py example ENH: Add parametric.py module, add plot_regresion_profiles to plot.py, and update plot_als_comparison.py example Jun 11, 2024
@arokem
Copy link
Collaborator

arokem commented Jun 14, 2024

@earoy : I just added you as a collaborator to this repo, so hopefully you won't have to wait for me to approve running the CI on each commit.

metric,
formula,
group="group",
lme=False,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick Q for now, but no time for a full review yet: Is it possible to tell from the formula whether this is an lme or not?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaning, is this argument redundant with information already provided in the formula?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've some digging through the statsmodels documentation and it looks like for mixed-effects models, the LME function takes an R-style formula for the fixed effects (which is identical to the OLS formula) and a groups parameter to specify the random effects, so I don't think we can tell from the formula....One thought though is to have the user pass in their model formula which we can then parse to determine whether it's OLS or MLE and then populate the call to the statsmodels LME function with the corresponding random effects structure if needed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid creating our own "domain specific language" and addressing all the possible corner cases may become pretty hairy, so maybe asking the user to be explicit about it (as you already do here) is best.

@coveralls
Copy link

coveralls commented Jun 14, 2024

Pull Request Test Coverage Report for Build 18de8b4d33257e1b712c59bc64d8a00d0d4a6dc2-PR-148

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build cbf13cf8f2660feceb8f57d5d1eb4b2c3e98b073: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@arokem
Copy link
Collaborator

arokem commented Jun 25, 2024

Looks like the CI error is unrelated to your changes: https://github.com/yeatmanlab/AFQ-Insight/actions/runs/9519578215/job/26243067587?pr=148#step:5:146, but instead comes from this line:

for legobj in leg.legendHandles:
, that you didn't even touch. I'll try to fix this here by pinning the version of Matplotlib as a stop-gap, so that we can make progress on this PR. Stand by.

@arokem
Copy link
Collaborator

arokem commented Jun 25, 2024

OK - I pushed this straight into your branch, so before you keep working here, please pull the recent commit from this branch to your local copy. Let's see if pinning Matplotlib fixes the error we saw in the CI before, and then we can proceed to review and merge this PR.

@coveralls
Copy link

coveralls commented Jun 25, 2024

Pull Request Test Coverage Report for Build ea525697945ac011e2cb21bfcc37087f0686adb0-PR-148

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build cbf13cf8f2660feceb8f57d5d1eb4b2c3e98b073: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@arokem
Copy link
Collaborator

arokem commented Jul 1, 2024

This will be fixed once this PR in copt is merged: openopt/copt#111 and we cut a new release of copt.

@arokem
Copy link
Collaborator

arokem commented Oct 7, 2024

Hey @earoy : this took a while to fix and now we have a new "upstream" fork in https://github.com/tractometry/AFQ-Insight. Do you have an interest in trying to resume this work with a PR there? If you are, I can give some guidance on how you would redirect this PR there.

@earoy
Copy link
Collaborator Author

earoy commented Oct 8, 2024

Hi @arokem, I'd love to keep working on getting this bundled into the new fork! Any guidance on how to properly do that would be most welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants