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

Rename variables #1145

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Rename variables #1145

merged 3 commits into from
Nov 20, 2024

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Nov 5, 2024

Related to #919. Tedana development's been pretty quiet lately, so I figured it's a good time to rename the variables we've wanted to rename for a while now.

Changes proposed in this pull request:

  • mmix --> mixing
  • mixm --> mixing_file
  • data_oc --> data_optcom
  • catd --> data_cat
  • comptable --> component_table
  • dd --> data_reduced

@tsalo tsalo added the refactoring issues proposing/requesting changes to the code which do not impact behavior label Nov 5, 2024
Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

I didn't see anything that would indicate this but I imagine the column names haven't changed in the tsv files, right?

That would make Rica break and I would have to release an update to look for that.

@tsalo
Copy link
Member Author

tsalo commented Nov 6, 2024

It should only rename internal variables. I avoided changing column names or command-line parameters. There's always a chance that I missed something though...

@eurunuela
Copy link
Collaborator

It should only rename internal variables. I avoided changing column names or command-line parameters. There's always a chance that I missed something though...

I don't think you did. But I wanted to double-check. Thanks, Taylor!

@tsalo tsalo requested a review from handwerkerd November 12, 2024 16:04
@tsalo
Copy link
Member Author

tsalo commented Nov 12, 2024

I think this PR merits two reviewers.

Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

LGTM.

One very minor thing I noticed ismmix is always mixing but ica_reclassify_workflow has mmix_orig while other places use mixing_orig. This is really minor since this is effectively a local temp variable and I don't think this will confuse anyone.

@tsalo
Copy link
Member Author

tsalo commented Nov 20, 2024

Thanks @eurunuela and @handwerkerd!

One very minor thing I noticed is mmix is always mixing but ica_reclassify_workflow has mmix_orig while other places use mixing_orig.

I just changed it. Once CI passes I'll merge.

@tsalo tsalo merged commit 7041e95 into ME-ICA:main Nov 20, 2024
14 checks passed
@tsalo tsalo deleted the rename-variables branch November 20, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring issues proposing/requesting changes to the code which do not impact behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants