Investigations

[T-Nicholls]

The conversion limits are about 70% of the way there, I'll probably finish them from home next week as it bothers me to leave them unfinished.

[coveralls]

Coverage decreased (-0.3%) to 99.473% when pulling f495603 on T-Nicholls:investigations into d06febc on dls-controls:master.

[willrogers]

In the very reasonable event that you do not decide to do this, can you tell me what you mean by 'about 70%'?

[T-Nicholls]

All the plumbing to get the limits onto the unit convs is done, and the convert method does check that the results are within the bounds. However, the _raw_phys_to_eng methods of the pchip and poly unit convs still apply their old checks to the results, according to the csv values, not the limits. So a value outside the bounds but inside the limits would be discarded, thus the convert method's check that the result is inside the limits is essentially useless as the conversion bounds are always smaller than the limits.

I think the fix for this is to:

• remove all checks from raw methods and just return a numpy array of all results.
• have all checking occur inside convert, rather than trying to split it across methods.
• if we have two results inside limits but only on is inside bounds, then use that one; otherwise, raise an error.

If you agree that this is the way to go about it then I'm happy to do it as it should be reasonably quick, I suspect that updating the tests will be the longest bit.

[willrogers]

Yes, I think that is correct. You should feel entirely free not to do this!

I also realised that set_post_phys_to_eng() etc. wasn't the right thing to do, we should have just made the attributes public. I will change this at some point.

[T-Nicholls]

It didn't integrate as smoothly as I hoped.

• I put everything in the convert method, as discussed, though I think it can be done more cleanly and needs more descriptive variable names, but it works.
• I removed eng_to_phys and phys_to_eng and put their functionality in convert too.
• I will update the tests on Wednesday/Thursday but thought I'd push what I've done so far to get any feedback.

[willrogers]

There's a couple of comments I'd make:

• id isn't really related to the functionality of UnitConv, it's just used to help debugging when something goes wrong. I'd be more inclined to set it as an attribute and use it in error messages if it's there than to require it every time you create such an object
• I'm not sure why you removed eng_to_phys() and phys_to_eng()` as I think they could contain the new functionality. Shorter methods are usually better than longer ones.

You absolutely shouldn't feel the need to continue working on this, I can pick it up.

[T-Nicholls]

I agree id shouldn’t be required and probably should be a private attribute anyway, just for use in error messages.
I felt the eng_to_phys and phys_to_eng methods were a bit redundant as we mostly used the convert method internally in pytac so I moved them to convert as it was only two lines; but I agree we could move all the new functionality there instead of convert and that would make more sense.