Confusions about checks in PolynomialTensor

[idk3]

I've been looking over PolynomialTensor after seeing some PRs touch it, and had a few questions. I'm not very familiar with the class, so I think one of them might be a misunderstanding, but the others appear to be problems that are a) not sufficiently tested and b) could give errors in the code.

My questions have to do with several places where a TypeError is raised, or tensors are considered unequal, if self.n_body_tensors.keys() != other.n_body_tensors.keys() in https://github.com/quantumlib/OpenFermion/blob/master/src/openfermion/ops/_polynomial_tensor.py.

The questions I have are

1. keys() doesn't guarantees order in its output, so I don't think these checks are particularly meaningful to begin with (they should at least use set(x.keys()) for comparison - if that's what we really want),
2. it's unclear to me that that should be what we want anyway - if a key exists in self but not in other with the corresponding value in self within EQ_TOLERANCE of zero, the tensors should be equal, not unequal (shouldn't they?),
3. TypeError says that arguments are the wrong type, not that something else is wrong with them. So TypeError is not the right error to be raising here. But as in 2. I don't think this condition should lead to an error at all.

Thoughts?

[kevinsung]

1. The keys() method of dictionaries actually returns a key view, which is a set-like object. See https://docs.python.org/3/library/stdtypes.html#dict-views.
2. EDIT: Okay actually I might agree with you. Maybe we should be able to compare and add PolynomialTensors that have different keys.
   However, this does lead me to another question: why is equality comparison here implemented with an __eq__ method, while for SymbolicOperators it is implemented with the isclose method? I don't see a reason to do it differently for both.
3. TypeError is generally raised to indicate that an operation is not meant to be performed in some way on some objects; it appears to me that this is how it's being used here. The returned error message can contain more specifics about the problem.

[idk3]

1. This is true in Python 3 but not in Python 2, see e.g. https://stackoverflow.com/questions/5629023/key-order-in-python-dictionaries .

2. This is a fair question in general - I don't have a good solution really. I agree that isclose is a better way to handle things rather than __eq__ at least for the purposes of consistency.

3. I think you're misinterpreting a point about functions and methods from the docs (I think you might be looking at the second paragraph of https://docs.python.org/3/library/exceptions.html#TypeError ?). The paragraph below clarifies this - "Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError." In this case the arguments would be of the correct type but incorrect value (the dictionaries x.n_body_tensors and y.n_body_tensors have different keys), and a ValueError or similar would be the appropriate error to raise.

[kevinsung]

1. We can handle this by adding the line from builtins import dict and initializing our dictionaries with dict(). I think that's the preferred way to use the future module to handle this compatibility issue.

Three. Okay, I agree that we should raise ValueError in some of these cases then.

[kevinsung]

@idk3 I made a new issue at #274 to implement the changes discussed here. Do you think that covers everything? If so I think we can close this issue.