Allow object arrays of mixed unit Quantities to be printed #3778
Conversation
…sn't create a new class but it does at least make arrays of Quantity objects printable without breaking or changing too much else. It adds a new UnitConversionError specifically for errors related to that, and is also a ValueError.
|
I think in principle |
|
I think I should add one more note in the changelog, that this changes the exception raised by unit conversion errors (though it's still a subclass of the original exception class used, so it shouldn't break any existing code). |
| @@ -282,6 +283,8 @@ def _get_time_fmt(self, val, val2, format, scale): | |||
| try: | |||
| return FormatClass(val, val2, scale, self.precision, | |||
| self.in_subfmt, self.out_subfmt) | |||
| except UnitConversionError: | |||
There was a problem hiding this comment.
I don't think the addition here is necessary. Really, one is just testing whether a format works at all, and a blanket except would arguably have been fine.
There was a problem hiding this comment.
I could change that, but I don't think I'm comfortable with a bare except here, at least for now. I agree it's a bit awkward though.
There was a problem hiding this comment.
I think it is fine just to remove the additional exception. I don't see how UnitConversionError could indicate anything else than "this format doesn't work, go on try the next".
There was a problem hiding this comment.
Ah, let me clarify, the reason this needs to be caught and handled separately is that there were some tests where a UnitsError was expected to be raised. Now a UnitConversionError is raised, but since that's a subclass of ValueError it needs to be handled separately.
I can't remember exactly what the use case was but there is code expecting this to raise unit conversion errors so that they can be handled separately.
There was a problem hiding this comment.
OK, yes, I think I now understand: that much be the interaction with Quantity in TimeDelta
There was a problem hiding this comment.
I double checked, and there are a few tests that fail without this line. For example TestTimeQuantity.test_invalid_quantity_input fails, because it tests instantiating a Time object from a quantity in units of meters. This should definitely result in a unit conversion error, instead of just being ignored (instead you end up getting an error "Input values did not match the format class jd", which is true in a sense, but it's not as useful as a unit error would be here.
|
@embray: on second review, one further small comment. As for the name, I'm still not entirely sure it is needed to introduce the new The travis failure is about an open file, which seems strange, given that nothing was changed that could lead to this. I was about to just restart that test, but then thought it might be good if you had a look, since it is such an odd failure. |
|
There are a few places where I'm not wedded to the overall solution here and could do away with |
|
Okay, since the build failure is just the |
Allow object arrays of mixed unit Quantities to be printed
Allow object arrays of mixed unit Quantities to be printed Conflicts: astropy/time/core.py
This partially addresses the issue raised in #3777 that printing/repr-ing an
ndarrayofdtype=objectcontaining quantities of different units resulted in a crash.I still think it would be nice to have a class specifically for this purpose as discussed in #3777, but this solves the immediate problem, and I will need this for further work.