Enable to add similar elements

[jkpnen]

Solves #194

[jkpnen]

Tests will not pass (after the latest commit e536dcd) if not sorted(orig_elems). Why is that and should it be sorted?

potku/tests/unit/test_element.py

Lines 64 to 82 in 96cf37b

	def test_lt(self):
	elems = [
	Element.from_string("H"),
	Element.from_string("He"),
	Element.from_string("C"),
	Element.from_string("C 2"),
	Element.from_string("4C"),
	Element.from_string("5C"),
	Element.from_string("15Si"),
	Element.from_string("Mn"),
	Element.from_string("Mn 2"),
	Element.from_string("16Mn"),
	Element.from_string("243Bk"),
	Element.from_string("250Cf")
	]
	orig_elems = list(elems)
	random.shuffle(elems)
	self.assertEqual(orig_elems, sorted(elems))

[tpitkanen]

These should be after the isotope comparisons, not instead of them.

[jkpnen]

Sorry, I do not understand what you suggest.

[jkpnen]

How is this different from this?

Similar to self.isotope:

Suggested change

	if self.sequence is None and other.sequence is not None:
	return True
	if self.isotope is not None and other.isotope is None:
	if self.sequence is not None and other.sequence is None:
	def __lt__(self, other):
	# ...
	# Elements that have no sequence number come before elements that do
	if self.sequence is None and other.sequence is not None:
	return True
	if self.sequence is not None and other.sequence is None:
	return False
	return str(self) < str(other)

Originally posted by @tpitkanen in #196 (comment)

[tpitkanen]

Elements were originally compared by member variables in this order:

1. symbol
2. isotope
3. (str conversion)

Now they are compared:

1. symbol
2. sequence
3. (str conversion)

But they should be compared:

1. symbol
2. isotope
3. sequence
4. (str conversion)

Also, I'm not sure if just comparing str(self) < str(other) at the end works now that there's self.sequence.

[tpitkanen]

Tests will not pass (after the latest commit e536dcd) if not sorted(orig_elems). Why is that and should it be sorted?

potku/tests/unit/test_element.py

Lines 64 to 82 in 96cf37b

	def test_lt(self):
	elems = [
	Element.from_string("H"),
	Element.from_string("He"),
	Element.from_string("C"),
	Element.from_string("C 2"),
	Element.from_string("4C"),
	Element.from_string("5C"),
	Element.from_string("15Si"),
	Element.from_string("Mn"),
	Element.from_string("Mn 2"),
	Element.from_string("16Mn"),
	Element.from_string("243Bk"),
	Element.from_string("250Cf")
	]
	orig_elems = list(elems)
	random.shuffle(elems)
	self.assertEqual(orig_elems, sorted(elems))

__lt__ is the less-than comparison. The new commit broke the isotope ordering/sorting as I noted here. The test should work as-is after the commented thing is fixed.

[tpitkanen]

I checked this pull request again while merging the open PRs and noticed the following:

• __init__: sequence defaults to 0 -> the sequence number is visible for each element
• Does the if element_str == 'SUM': check work for the new style of sum spectra?
• __str__ is not updated
  • there is a corresponding from_string method, so the following must always hold: elem == Element.from_string(str(elem))
  • changing this will probably cause issues in places where it's used
  • the __lt__ comparison method relies on this to compare elements that both have a sequence
    • comparison is currently subtly broken: __eq__ correctly compares non-None sequences, but __lt__ doesn't
• if self.symbol is None and other.symbol is not None: and if self.symbol is not None and other.symbol is None: are not needed because there's already if self.symbol != other.symbol: above them
  • even if the check weren't there, the symbol parameter is mandatory so there shouldn't be a situation where it's None
• Outdated docstring:
  • __init__
  • __str__
  • __lt__
  • get_prefix

---

More importantly, I reconsidered the risk/reward ratio of this feature. While being able to add similar elements is useful, the Element class is used in many places (around 100 non-test locations according to Find Usages, of which from_string accounts for 24 occurrences). Any of these places could break with this change.

Even if the code doesn't break, the effects of the new sequence member variable need to be considered in each of the places Element is used.

I don't think this feature should be added after all. It's too much work and code complexity for its benefits.

(Ping for the corresponding issue: #194)

[tpitkanen]

@mlaitin: Just two types of same element would be enough (SCT/REC).

This feature should be simpler/safer to implement this way.

#194