Skip to content

Chore: Convert variables to lowercase #157

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

Closed

Conversation

john-halloran
Copy link

This PR is not fully finished, but I wanted to get started with the review to make sure the style is acceptable. A few variables within functions still need to be renamed.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

yes, this is in the right direction. Is that intended that you deleted all those other py files?

If this is a "cleaning" operation, then I would put that on a separate commit with nothing else on it that is clearly labelled so if we want any of those files back in the future we know we can get them from the commit before the cleaning commit.

@@ -7,8 +7,8 @@
Y0 = np.loadtxt("input/W0.txt", dtype=float)
N, M = MM.shape

my_model = SNMFOptimizer(MM=MM, Y0=Y0, X0=X0, A0=A0)
my_model = SNMFOptimizer(source_matrix=MM, init_weights=Y0, init_comps=X0, init_stretch=A0)
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, much more readable. But you still have some upper-cased variables there (MM, Y0 etc.). It is not clear what these are defaulting to. Should they not be positional variables and not optional variables? What happens in they are not all specified? Only make them optional if the function can be run successfully when one is not sepcified.

Copy link
Author

Choose a reason for hiding this comment

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

Will close this to separate out the cleaning.

@john-halloran john-halloran deleted the lowercasing branch June 19, 2025 00:51
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.

2 participants