AguaClara / aguaclara

An open-source Python package for designing and performing research on AguaClara water treatment plants.
https://aguaclara.github.io/aguaclara/
MIT License
24 stars 13 forks source link

Errors with head_loss.py and design/test_lfom.py #103

Closed HannahSi closed 5 years ago

HannahSi commented 5 years ago

The @ut.list_handler calls are updated now to @ut.list_handler() (thank you @fletchapin for the function decorator fix!) However, there are still a few lingering errors. Does anybody have some insight on these?

A call on k_value_reduction() in tests/core/test_headloss.py gives ValueError: A wrapped function using strict=True requires quantity for all arguments with not None units. (error found for meter, 0.16827499999999998)

Line 11 from tests/design/test_lfom.py gives AttributeError: module '[secure].core.pipes' has no attribute 'SDR_available_ND'

Also, I saw that all of tests/core/test_lfom.py was commented out because it tested old LFOM functions. Is tests/design/test_lfom.py also old? And would it be alright to delete the test_lfom.py in the core folder altogether?

fletchapin commented 5 years ago

@HannahSi the strict=True error is part of pints unit wrapper. I believe I explained it before but in general we should set all functions to strict=false in the wrapper so that we don’t have to worry about checking units on dimensionless constants

fletchapin commented 5 years ago

The other error I’ll have to take a closer look at later in the week when I’m less busy

eak24 commented 5 years ago

You can delete the test_lfom file that is in the core folder.

On Sat, Oct 27, 2018 at 10:24 PM Fletcher Chapin notifications@github.com wrote:

The other error I’ll have to take a closer look at later in the week when I’m less busy

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/AguaClara/aguaclara/issues/103#issuecomment-433670850, or mute the thread https://github.com/notifications/unsubscribe-auth/AIMAo93VXo5ImyiAOcz8ppRWIapiqy08ks5upRVAgaJpZM4X9sGU .

-- Software Engineer, Onshape

HannahSi commented 5 years ago

Got it, I added a strict=False argument in the function wrappers for k_value_expansion and k_value_reduction. The error about a wrapped function disappeared, but k_value_orifice is still acting strangely when it enters the "oversize" case:

Here's the error k_value_orifice raises at line 42 of test_head_loss.py:

self.assertAlmostEqual(k.k_value_orifice(pipe.OD(6), pipe.OD(4), 60*u.inch, 1 * u.L / u.s), 1.8350488368427034) AssertionError: <Quantity(1.8577290828680864 dimensionless, 'dimensionless')> != 1.8350488368427034 within 7 places`

I did some testing, and the return statement in the k_value_orifice function

return k_value_reduction(pipe_id, orifice_id, q) \
      + k_value_expansion(orifice_id, pipe_id, q))

should be equivalent to something like

return k_value_reduction(pipe.OD(6), pipe.OD(4), 1 * u.L / u.s) \
      + k.k_value_expansion(pipe.OD(4), pipe.OD(6), 1 * u.L / u.s)

(for the call that gave the error). But the first block of code prints "1.858 dimensionless dimensionless", and the second "1.835 dimensionless".

We can get past the problem of inconsistent values, but why is an extra dimensionless unit appearing, and only in the oversize case for k_value_orifice?

HannahSi commented 5 years ago

@fletchapin The second issue, with lfom.py, has been resolved! It turned out it was caused by a typo in a variable name.

fletchapin commented 5 years ago

@HannahSi I'll take a look and try to reproduce the issue you're seeing with dimensionless units. After I'll update my findings here

oliver-leung commented 5 years ago

All tests are passing now - we decided to omit lfom.py tests that referenced onshapepy for now, since we weren't sure how to make it work with the .onshapepy file while passing Travis tests. @ethan92429 how did you make it work initially?

We've made pull request #109 to merge to master.

eak24 commented 5 years ago

Hey @oliver-leung , what's the error? When did it start happening? How did the change relate to how onshapepy.enc works? Were the tests passing before, and if not now, why?

oliver-leung commented 5 years ago

The wrapper issues are being worked on in #205 and the AppVeyor issues are being worked on in #217