Standardize test names: e.g. LatticeTestCase -> TestLattice

[janosh]

PR was completely changed after discussion below. now just minor stuff like formatting and better variable names

[janosh]

curious if anyone has thoughts on if we really want this behavior and if so, can you think of more edge cases to test. @JaGeo @Andrew-S-Rosen @shyuep @DanielYang59

[JaGeo]

Mh. I feel like in the composition case, this might be a bit too much as a composition would also include floats and ints to specify the exact composition. This could get really messy.

[JaGeo]

For species, I agree that this would be useful but I am not so sure about composition.

[janosh]

@JaGeo thanks for chiming in!

i should have mentioned the initial motivation for this change came from me noticing (and being surprised) that Structure already handles int (e.g. si = 14 below):

pymatgen/tests/io/vasp/test_inputs.py

Lines 383 to 392 in 48af038

	si = 14
	coords = [[0, 0, 0], [0.75, 0.5, 0.75]]
	# Silicon structure for testing.
	latt = [
	[3.8401979337, 0.00, 0.00],
	[1.9200989668, 3.3257101909, 0.00],
	[0.00, -2.2171384943, 3.1355090603],
	]
	struct = Structure(latt, [si, si], coords)

at that point it seems natural to expect as a user that other core classes like Composition should do the same. at least that's the first thing i tried when i noticed Structure(species=[14]) is supported

i'm not so much worried about the code getting messy. a single int/float was easy to handle in the Composition constructor. i'm more worried user expectations for what Composition(1) should return could be inconsistent? but then i can't see what other behavior one might expect

[JaGeo]

I see. If this is only implemented for single ints or floats, I think the intention should be quite clear. Thus, I agree. This should both be fine.

[DanielYang59]

Thanks for pinging me here 😄 .

But for me personally I think initialize a Species or Composition with an atomic number feels a bit weird/unexpected...Because I think people are expecting chemical Species/Composition to look "chemical" like Composition("LiFePO4")

Also init a Species/Composition might be limited to single-atom Species/Composition, and this might be a very limited use case.(How to init a Species/Composition like "H2O" from int for example?)

I assume a helper func to convert int to element name would be more straightforward, and could be reused universally instead of modifying all these classes to accept int?

[shyuep]

I vote no on this change. I don't even understand why this is needed.

[janosh]

I don't even understand why this is needed.

it's easy to imagine use cases where people have integers and want to create Species from them without interconverting to symbols.
also, did you read my comment?

the initial motivation for this change came from me noticing (and being surprised) that Structure already handles int

at that point it seems natural to expect as a user that other core classes like Composition should do the same

@shyuep given your question i have a counter question: why do we need atomic number support in Structure?

[shyuep]

@janosh Given that no one has ever complained about Composition supporting int/float to my knowledge, I would imagine the use case is theoretical and not practical. Composition(1) is ambiguous and for sure, this will not be something I agree to. Species(1) is slightly more justifiable but given that the pseudo-parent class Element does not support Element(1), this is again extremely bad behavior.

Structure supporting int has good reasons. There are some electronic structure codes where the sites are specified in terms of atomic numbers. Also, for ordered structures, the int specification has a 1:1 link with an atom, this is not ambiguous.

I would ask that we solve actual problems that people faced, not imagine problems and add complexity to the code to handle a theoretical use case that no one has ever raised.

[janosh]

@shyuep please stop closing this PR. it contains other changes that should be merged regardless of whether we go ahead with the Composition/Species change. depending on the outcome here, i'll revert Compositon changes and merge the rest.

add complexity to the code to handle a theoretical use case that no one has ever raised.

the grand total of added complexity are 2 lines. this is all it takes to support int/float in Composition

if len(args) == 1 and isinstance(args[0], (int, float)):
    # treat single integer/float as atomic number
    elem_map = {get_el_sp(args[0]): 1}

and then 2 more lines to test it, i.e. negligible

Species(1) is slightly more justifiable but given that the pseudo-parent class Element does not support Element(1), this is again extremely bad behavior.

that is a shortcoming of Element and should be changed. it's not a reason to not support int in Species

[shyuep]

There is a good reason why Element does from_Z and not Element(1). It is an Enum and was done for backward compatibility.

A PR should contain the changes that it is supposed to have - no more, no less. If you want to keep the PR open, by all means but the title should be changed to reflect the actual changes that this PR is supposed to do. But supporting int in Species and Composition is not necessary and adds ambiguity. Like I said - find me someone who has ever complained about the lack of this "feature" and we can discuss it.

[janosh]

It is an Enum and was done for backward compatibility.

yeah i saw the comment. though i don't understand what would break by supporting Element(1). do you remember the details? if not, could be worth another try.

the title should be changed to reflect the actual changes

will do

find me someone who has ever complained about the lack of this "feature" and we can discuss it

myself actually. i'm not making this up. the first time i used Element years ago I tried Element(8) to get oxygen which of course didn't work so i had to search the code to find Element.from_Z

[shyuep]

It has always been that an Element is initialized by Element("H"). While you can obviously use an "isinstance" to support int, it is not really necessary.