Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions news/use-valuerror.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* <news item>

**Changed:**

* Use ``ValueError`` instead of ``TypeError`` for failed ``Operator.operation``.

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
8 changes: 7 additions & 1 deletion src/diffpy/srfit/equation/literals/operators.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,13 @@ def getValue(self):
"""Get or evaluate the value of the operator."""
if self._value is None:
vals = [arg.value for arg in self.args]
self._value = self.operation(*vals)
try:
self._value = self.operation(*vals)
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be TypeError maybe? Turning any exception into a valueError may lead to confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. After a second thought, I think maybe we should not modify it at all.

The test-case-bad uses np.add but only gives it one argument. We want to use ValueError instead of TypeError in this bad case, because the input type is correct. But we can't customize all possible Errors that a user might encounter in their practice. I think the best approach is to leave the original error message and let the user see where they are wrong directly.

So, I think it is better to raise whatever the external function raises. In this case, we can close #72 as not planned.

raise ValueError(
"Error evaluating operator '%s' with arguments %s : %s"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we have this as an f-string for greater readability?

% (self, vals, e)
)
return self._value

value = property(lambda self: self.getValue())
Expand Down
4 changes: 2 additions & 2 deletions tests/test_literals.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def testAddLiteral(self):
"""Test adding a literal to an operator node."""
op = self.op

self.assertRaises(TypeError, op.getValue)
self.assertRaises(ValueError, op.getValue)
op._value = 1
self.assertEqual(op.getValue(), 1)

Expand All @@ -124,7 +124,7 @@ def testAddLiteral(self):
b = literals.Argument(name="b", value=0)

op.addLiteral(a)
self.assertRaises(TypeError, op.getValue)
self.assertRaises(ValueError, op.getValue)

op.addLiteral(b)
self.assertAlmostEqual(0, op.value)
Expand Down
2 changes: 1 addition & 1 deletion tests/test_visitors.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ def testSimpleFunction(self):
assert plus2.hasObserver(mult._flush)

# plus2 has no arguments yet. Verify this.
with pytest.raises(TypeError):
with pytest.raises(ValueError):
mult.getValue()
# Add the arguments to plus2.
plus2.addLiteral(v4)
Expand Down
Loading