smiths / caseStudies

Case studies of (manual) documentation for scientific computing software
3 stars 2 forks source link

Add Exceptions to Implementation #81

Closed samm82 closed 6 years ago

samm82 commented 6 years ago

The original Python directory removed in 80250c2 had exceptions to check for invalid calculated values, while its replacement (formerly Python_Simplified) does not. We need to add these exceptions to the implementations, for example:

raise SystemExit("Error: q_hat(DD6 in the SRS) is out of bounds. q_hat is less than the smallest value in q_vec. The input a, b might be too small while t might be too large. Please refer to the data definitions section and the data constraints section in the SRS.")

in calculations.py.


TODO:

samm82 commented 6 years ago

After a3e1bac, running make test spits out this error:

======================================================================
ERROR: test_check_constraints (testCheckConstraints.TestCheckConstraints)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\samcr\Desktop\Summer Research\caseStudies\CaseStudies\glass\Implementations\Python\Test\testCheckConstraints.py", line 21, in setUp
    derivedValues.derived_params(self.params[i])
  File "C:\Users\samcr\Desktop\Summer Research\caseStudies\CaseStudies\glass\Implementations\Python\Implementation\derivedValues.py", line 43, in derived_params
    raise ValueError("Wrong thickness entered - Not an industrial standard")
ValueError: Wrong thickness entered - Not an industrial standard

despite the value in defaultInput.txt being "10.0" - which is a valid input.

There is one input file that has an intentionally wrong input, but I'm not sure if the test cases are looking for it.

EDIT: This is thrown by the wrong input - the problem is that it's not expected - working on it.

smiths commented 6 years ago

Test cases that test whether an expected exception is generated are also valid test cases. I do not remember the syntax, but your assertion for this test case will be that a ValueError exception is raised when you expect one.

samm82 commented 6 years ago

So the exception I added is already checked for, which is probably why the function in derivedValues.py used print statement and not an exception. It's more of a formality/extra warning. The actual exception (which is more informative anyway) is:

https://github.com/smiths/caseStudies/blob/a3e1baca454981bc7ce9ec8c8a40119c64cab22d/CaseStudies/glass/Implementations/Python/Implementation/checkConstraints.py#L10-L11

I'm going to revert the change I made to the print statement.

smiths commented 6 years ago

Is the print statement warning message even reachable code? If the exception is caught earlier, won't the program exit? Should we be testing for the exception twice? We should probably be thinking about the big picture design of this program. Something seems off here.

samm82 commented 6 years ago

Potentially? I mean I wouldn't mind if I was coding something, but I've also never coded anything this big picture or important. 😆 I think it is good to have a "catch-all" in the conditionals in derivedValues.py just to have, but I could be totally wrong.

samm82 commented 6 years ago

@smiths The exception I used in the example is in this context: https://github.com/smiths/caseStudies/blob/0805bc19a85a149d355b8992481782fd3e09a977/CaseStudies/glass/Implementations/Python/Implementation/calculations.py#L49-L50 However, Python_Simplified never calculates a q_vec (I couldn't find it with grep). Is that a problem/do we need to implement a q_vec calculation?


One more thing - the only exceptions in the original Python folder were in calculations.py. What was your intent for this issue? Was is just that I was going to go through and copy over lost exceptions, or to actually create exceptions for values that aren't acceptable to GlassBR? Would I find these in the SRS?

smiths commented 6 years ago

q_vec is used in calculations.py. It is a local variable for that module. Specifically it is used for the calculation of J. The source of q_vec in the notation is the hand written documentation that I did for Steven Palmer:

https://github.com/smiths/caseStudies/blob/master/CaseStudies/glass/Documentation/MIS/MathModelForCalc_q.pdf

Following the handwritten note approach to the calculations, q_vec is definitely necessary. (In the hand written notes an underline is used to indicate a vector. You might not have previously run across this notation.)

As far as Exceptions go, the intent was to replace print statement error messages with Exceptions. If you have done that, we can close this issue.

We should look at the design of GlassBR. The MG and MIS are in need of revision. The design should actually be quite simple; based on the code implementation, it seems more confusing than it should be. The syntax of the functions in the new design may include Exceptions that we don't currently have, but we should start with the design for determining the exceptions, rather than the code.

Can you please create an issue and assign it to me for revision of the design (MG and MIS) for GlassBR?

samm82 commented 6 years ago

derivedValues.py currently raises ValueErrors instead of print statements, but this code shouldn't really be run ever, since the inputs are checked in checkConstraints.py. I don't think it's a big concern (I can remove them if you want me to @smiths), so I think this issue is closed.

smiths commented 6 years ago

Yes, we can close this issue. We may reopen it, or create something similar when I go through issue #89.