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

Documentation for AmpAndLengthScale mentions internal attributes #32

Open
pmeier opened this issue Oct 3, 2021 · 3 comments
Open

Documentation for AmpAndLengthScale mentions internal attributes #32

pmeier opened this issue Oct 3, 2021 · 3 comments

Comments

@pmeier
Copy link

pmeier commented Oct 3, 2021

If amplitude and _length_scale should be public, we should remove the leading underscore. Otherwise they should be removed from the documentation.

@a-ws-m
Copy link
Owner

a-ws-m commented Oct 14, 2021

I think this is an interesting issue and I'm not sure what the best approach is. On the one hand, I don't think those attributes should be public; they should only be referenced within the class (or classes that inherit from AmpAndLengthScale, rather). On the other hand, I think it is useful to keep them in the documentation, as it relates to extending the API with one's own kernels, and they're necessary to interface with if you wish to do so. Any recommendations?
EDIT: My confusion might be based on a lack of intuition about what strictly ought to be "private".

@pmeier
Copy link
Author

pmeier commented Oct 15, 2021

I've had quite a few discussion about this and I'm afraid there is no right answer here. My recommendation is the following: every public attribute or method of a class, i.e. no leading underscore, should have a proper use case for someone using that class. This also entails that the functionality is documented. Your case might be a little special due to the fact that we are talking about an ABC here.

I think it comes down to a matter of style, so I don't consider this a blocker. Feel free to close this with or without "fix".

a-ws-m added a commit that referenced this issue Oct 22, 2021
@a-ws-m
Copy link
Owner

a-ws-m commented Oct 22, 2021

That makes sense, and thinking about this made me decide to redesign this part of the code. I think calling them amplitude and length_scale in the form that they were in is probably misleading because they weren't the actual amplitude and length scale used to parameterise the kernel; they were precursors that were passed through a different function, first. I've moved this functionality into amplitude and length_scale properties that are now documented.
I've decided to keep the newly renamed _amplitude_basis and _length_scale_basis attributes in the docs, however, as I think they're useful to know about for people who want to extend the API.

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

No branches or pull requests

2 participants