JuliaMolSim / AtomsBase.jl

A Julian abstract interface for atomic structures.
https://juliamolsim.github.io/AtomsBase.jl/
MIT License
84 stars 18 forks source link

Rename bounding_box to cell_vectors consistently #127

Open mfherbst opened 3 weeks ago

mfherbst commented 3 weeks ago

Renamed occurrences of cell_vectors to bounding_box for consistency.

cortner commented 3 weeks ago

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 commented 3 weeks ago

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

@jgreener64 @rkurchin others thoughts ?

rkurchin commented 3 weeks ago

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 commented 3 weeks ago

No opinions beyond consistency.

cortner commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

mfherbst commented 3 weeks ago

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

mfherbst commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 5 days ago

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 commented 5 days ago

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