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 bounding_box to cell_vectors consistently #127

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

mfherbst
Copy link
Member

Renamed occurrences of cell_vectors to bounding_box for consistency.

@cortner
Copy link
Member

cortner commented Oct 25, 2024

I disagree with this change. cell_vectors is more typical than bounding_box and in fact I would remove bounding_box in favour of cell_vectors.

@mfherbst
Copy link
Member Author

Fine with me as long as it's consistent in the code base.

@jgreener64 @rkurchin others thoughts ?

@rkurchin
Copy link
Collaborator

It seems that the core point here is that we should be consistent about what we call this thing, which I definitely agree with. I personally do not have strong feelings about which particular name we go with.

@jgreener64
Copy link
Collaborator

No opinions beyond consistency.

@cortner
Copy link
Member

cortner commented Oct 28, 2024

not sure what to say here, I stand by what I said. google "bounding box" and "cell vectors" and see what you get. Bounding box is a concept from image processing, while cell vectors immediately brings up links to Wikipedia Unit Cell.

If you all disagree with me and decide you prefer to stick with the existing terminology, then not much I can do.

If you want to consider changing the terminology, then the natural thing to do would be to deprecate the bounding_box function for the next minor version.

@tjjarvinen
Copy link
Collaborator

I would prefer cell_vectors also. It is a much better name, as it does not need explanations. (plus what Christoph mentioned about googling)

@jgreener64
Copy link
Collaborator

To clarify I am fine with that change as long as the same term is used throughout the codebase.

@mfherbst
Copy link
Member Author

mfherbst commented Oct 28, 2024

Same here. If no one expresses clear preference in the other direction, I'd suggest we change everything to cell_vectors.

@mfherbst mfherbst changed the title Remove cell_vectors kwarg which is not consistent Rename bounding_box to cell_vectors consistently Nov 2, 2024
@mfherbst
Copy link
Member Author

mfherbst commented Nov 2, 2024

Ok, this will now completely remove bounding_box in favour of cell_vectors.

How do we feel about the plural form here. I used it because of our previous discussion. But now I note that cell_vectors is potentially inconsistent with our other functions and keyword arguments, which are all singular.

I'm not too sure about this. Singular is a bit strange here, too.

What are your opinions ?

@rashidrafeek
Copy link
Contributor

I think the plural form cell_vectors is fine as it is a common terminology.

Also just pointing out that this is technically breaking as bounding_box was not removed from the interface in the v0.4 update and is still mentioned as part of the interface. It doesn't affect me personally, as I have not yet shifted to 0.4, so no issues if we decide to remove it. Would it be better to just deprecate bounding_box and remove it in 0.5?

@cortner
Copy link
Member

cortner commented Nov 3, 2024

Technically we can use cell_vector(sys, :) but I agree this is strange in this context.

Using the plural is not inconsistent because cell_vectors is a system property and not a particle property.

I strongly support plural.

(and yes to the last post - this is breaking and will have a deprecation cycle.)

@rkurchin
Copy link
Collaborator

I'm good with this being merged once the docs issue is fixed. I would suggest we add a minor version bump here too though so we can start having the deprecation warnings etc.

@mfherbst
Copy link
Member Author

Sure, I agree. Should get to it later today.

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.

6 participants