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 common variables and functions #448

Closed
tsalo opened this issue Nov 10, 2019 · 12 comments
Closed

Rename common variables and functions #448

tsalo opened this issue Nov 10, 2019 · 12 comments
Labels
discussion issues that still need to be discussed effort: medium Theoretically <40h total work impact: low Improving code/documentation cleanliness/clarity, not function priority: low issues that are not urgent refactoring issues proposing/requesting changes to the code which do not impact behavior
Milestone

Comments

@tsalo
Copy link
Member

tsalo commented Nov 10, 2019

Summary

There are a number of variables and functions that should be renamed across the package, in my mind, because they are confusing or uninformative.

A partial list

  • catd --> data_cat
  • tsoc --> data_optcom
  • masksum --> adaptive_mask
  • t2s --> t2s_limited
  • s0 --> s0_limited
  • t2ss --> t2s_ascending
  • s0s --> s0_ascending
  • t2sG --> t2s_full
  • s0G --> s0_full
  • dd --> data_dimred
  • mmix --> mixing (sklearn convention)
  • betas --> betas_optcom
  • tedana.decomposition._utils.eimask() --> outlier_mask()
  • tedana.stats.computefeats2() --> z_comp_maps()
  • tedana.io.writeresults() --> tedana.io.write_results()
  • tedana.io.writefeats() --> tedana.io.write_feats()
  • tedana.io.load_data() --> tedana.io.reshape_data() (since it just reshaped input 4D data to 2D/3D)
  • tedana.gscontrol.gscontrol_mmix() --> tedana.gscontrol.minimum_image_regression()
  • tedana.gscontrol.gscontrol_raw() --> tedana.gscontrol.gscontrol()
  • Finally, how do we feel generally about betas vs. parameter estimates vs. regression parameter, etc? I personally had enough classes with SPSS that I automatically interpret "beta" as "standardized parameter estimate", which is not how we use it throughout the package.

Next Steps

  1. Discussion.
  2. Large-scale renaming.
@tsalo tsalo added discussion issues that still need to be discussed refactoring issues proposing/requesting changes to the code which do not impact behavior labels Nov 10, 2019
@dowdlelt
Copy link
Collaborator

I'm fine with betas as the label and it is nice and short. do we think it should include that these are ICA betas? As in, betas_ica_optcom? If the optcom was not used to generate the betas, would it be betas_sepechoes? or something like that?

Overall I am very much pro renaming to names that kind of mean something. Just reading this has helped me make sense of it.

@emdupre
Copy link
Member

emdupre commented Nov 13, 2019

What do you think of ctab to comp_tbl ?

@rmarkello
Copy link
Member

@emdupre: I think @handwerkerd had updated ctab to just comptable in his branch of the code, which would also work!

@tsalo
Copy link
Member Author

tsalo commented Nov 13, 2019

ctab refers to the comptable file provided by the user though, while comptable is the DataFrame. I think there are times when we need separate variables. Same for the mixing matrix and T2* map (pending #408).

@tsalo
Copy link
Member Author

tsalo commented Dec 12, 2019

I'd like this to be the final thing we address for 0.0.9.

@stale
Copy link

stale bot commented Mar 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to tedana:tada: !

@stale stale bot added the stale label Mar 11, 2020
@tsalo
Copy link
Member Author

tsalo commented Mar 13, 2020

Not stale.

@stale stale bot removed the stale label Mar 13, 2020
@tsalo
Copy link
Member Author

tsalo commented Apr 16, 2020

I propose we push this to 0.0.10.

@tsalo tsalo modified the milestones: 0.0.9, 0.1.0 Apr 20, 2020
@tsalo
Copy link
Member Author

tsalo commented May 22, 2020

I'd like to start working on this in conjunction with #146 soon, given that there are relatively few active open PRs with major changes to the code. (My assumption is that the reports PR doesn't have much overlap with the files that need to be changed).

However, these changes will lead to major merge conflicts for any open pull requests, as well as conflicts for the proposed changes if new PRs are opened while this one is still unmerged, so I want to see what people think. I'd prefer to fast-track review of the proposed PR so that it doesn't get stuck and have conflicts accumulate.

Alternatively, I could break the changes into a bunch of separate PRs to keep the diffs manageable. What are folks' thoughts?

@jbteves
Copy link
Collaborator

jbteves commented May 22, 2020

My 2 cents:
I think we'll want to do one major PR and get it merged quickly. Every time we merge it'll make a downstream series of merge conflicts, so we want to minimize the number of merges. So we should do one merge and plan that after merge, we should coordinate to help people resolve conflicts on their forks.

@stale
Copy link

stale bot commented Aug 21, 2020

This issue has been automatically marked as stale because it has not had any activity in 90 days. It will be closed in 600 days if no further activity occurs. Thank you for your contributions to tedana:tada: !

@stale stale bot added the stale label Aug 21, 2020
@tsalo tsalo removed the stale label Feb 5, 2021
@tsalo tsalo added effort: medium Theoretically <40h total work impact: low Improving code/documentation cleanliness/clarity, not function priority: low issues that are not urgent labels Mar 22, 2021
@tsalo
Copy link
Member Author

tsalo commented Dec 23, 2022

Happy to close this in favor of #919.

@tsalo tsalo closed this as not planned Won't fix, can't repro, duplicate, stale Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion issues that still need to be discussed effort: medium Theoretically <40h total work impact: low Improving code/documentation cleanliness/clarity, not function priority: low issues that are not urgent refactoring issues proposing/requesting changes to the code which do not impact behavior
Projects
None yet
Development

No branches or pull requests

5 participants