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

make them private #117

Closed
wants to merge 8 commits into from
Closed

make them private #117

wants to merge 8 commits into from

Conversation

fnrizzi
Copy link
Member

@fnrizzi fnrizzi commented Jan 30, 2024

No description provided.

@fnrizzi fnrizzi linked an issue Jan 30, 2024 that may be closed by this pull request
@jtencer
Copy link
Contributor

jtencer commented Jan 30, 2024

I'm fine with this change, but I think what it accomplishes is actually to make these methods public but in a different module. Rather than romtools.trial_space.tensor_to_matrix we now have romtools.tiral_space.utils.tensor_to_matrix. If we want to indicate that it's a private function, we should prepend an underscore to the name and place the function in the module that uses it.

@fnrizzi
Copy link
Member Author

fnrizzi commented Jan 30, 2024

I thought that the distinction private public for us is just what is documented vs what is not.
These functions are omitted from being documented in pdoc.

I am happy to prepend underscore but that still does not make private :)

@jtencer
Copy link
Contributor

jtencer commented Jan 30, 2024

"python private" I guess. There's not real privacy in python, but prepending an underscore prevents them from being imported with from romtools.trial_space.utils import * and is generally the convention for "keep out". I haven't used pdoc, but at least for sphinx and underscore has the same effect of omitting it from the documentation. Does moving them to utils only omit them from the documentation because we don't have any documentation for utils?

@jtencer
Copy link
Contributor

jtencer commented Jan 30, 2024

Did some googling and it looks like pdoc ignores methods that start with _

I don't see anything in the docs which would make utils explicitly private.

@jtencer
Copy link
Contributor

jtencer commented Jan 30, 2024

To clarify, I think it's OK to have these functions in utils and to have them be public but undocumented. I also think it's OK to make them private in the sense of adding an underscore. However, I do not think that we should have them in utils and have them be private because having them in utils requires that they be imported and used outside of the scope in which they're defined. If we make them private, we should make them private to the module in which they're used.

Does that make sense?

@eparish1
Copy link
Contributor

I agree about prepending with the _. I think if we make these changes we are good.

@fnrizzi
Copy link
Member Author

fnrizzi commented Jan 31, 2024

@jtencer @eparish1 free functions with prepended __ do not work. That works only for class methods.
So i opted to use a trailing _impl. I hope this is fine

@fnrizzi
Copy link
Member Author

fnrizzi commented Jan 31, 2024

so @jtencer do you want me to make them private inside the file where they are used or leave them where they are ?

@fnrizzi fnrizzi requested a review from jtencer February 1, 2024 14:26
@jtencer
Copy link
Contributor

jtencer commented Feb 1, 2024

I'm good with it now

@eparish1
Copy link
Contributor

eparish1 commented Feb 1, 2024

@fnrizzi please ping me if you'd like me to resolve the conflict.

@fnrizzi
Copy link
Member Author

fnrizzi commented Feb 1, 2024

superseeded by #122

@fnrizzi fnrizzi closed this Feb 1, 2024
@fnrizzi fnrizzi deleted the make_ttm_private_111 branch February 21, 2024 07:54
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.

public vs private tensor_to_matrix(tensor_input)
3 participants