Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continue to Improve Testing for GlassBR #70

Closed
niazim3 opened this issue May 9, 2018 · 10 comments
Closed

Continue to Improve Testing for GlassBR #70

niazim3 opened this issue May 9, 2018 · 10 comments
Assignees

Comments

@niazim3
Copy link
Collaborator

niazim3 commented May 9, 2018

This issue is a continuation of JacquesCarette/Drasil#347.

testInterp.py

Broken as Implementation module has no attributes find_bound and interp and x1 is not defined (mentioned in comments of this commit).

testReadTable.py

Broken, as mentioned in the comments of this commit.

testMainFun.py

Has 1 failure upon running make test:
image

Please update TestingPythonGlassBR.tex and TestingPythonGlassBR.pdf as deemed necessary.

@niazim3 niazim3 self-assigned this May 9, 2018
@niazim3
Copy link
Collaborator Author

niazim3 commented May 14, 2018

Note: interp.py in Python subfolder defines

def lin_interp(y1, y2, x1, x2, input_param):
...

def proper_index(index1,index2,data,value):
... 

# find_bounds: numpy.ndarray numpy.ndarray Num Num -> Int Int Int Int Int
def find_bounds(data1, data2, value1, value2):
...

# interp: Int Int Int Int Int Int numpy.ndarray numpy.ndarray numpy.ndarray Num Num -> Num
def interp(idx, jdx, kdx, num_interp1, num_interp2, data1, data2, data3, value1, value2):
...

was simplified to
interp.py in Python_Simplified subfolder, which defines

class BoundError(Exception):
...

def lin_interp(x1, y1, x2, y2, x):
...

# arr => Sj (i.e. sequence of numbers representing possible standOff distances)
# v   => SD calculated (i.e. number)
def indInSeq(arr, v):
...

def matrixCol(mat, c):
...
    
def interpY(x_array, y_array, z_array, x, z):
...
        
def interpZ(x_array, y_array, z_array, x, y):
...

@niazim3

This comment has been minimized.

@smiths

This comment has been minimized.

@niazim3
Copy link
Collaborator Author

niazim3 commented May 15, 2018

Note to self for cleanup of infrastructure (WIP)

  • For INTERPOLATION TESTING, /Test contains testInterp.py which is a file that parameterizes the hard coded inputs in the /OldTests testInterp{#}.py (# = '' to 14); all the comments from these files were input under the column heading of "Significant inputs" in table 5 of the TestingPythonGlassBR.pdf
    (which means the files in /OldTests can be removed after

  • ensuring the comments in the documentation pdf is correct

  • inputs in interp.txt were copied over correctly)

  • For INTERPOLATION IMPLEMENTATION, as mentioned in Test Documentation for GlassBR JacquesCarette/Drasil#262, Python_Simplified contains the "improved" from Python and that "The code is correct, it is the tests that are not.", which means new/updated test cases are required for the functions matrixCol(mat, c), interpY(x_array, y_array, z_array, x, z), and interpZ(x_array, y_array, z_array, x, y) (as they're replacing find_bounds(data1, data2, value1, value2) and interp(idx, jdx, kdx, num_interp1, num_interp2, data1, data2, data3, value1, value2))

  • For READTABLE TESTING, currently broken due to the following error showing up upon running make test :

AttributeError: 'bool' object has no attribute 'all'

, which is related to the implementation

  • For READTABLE IMPLEMENTATION, 3 functions (read_z_array(filename), read_x_array(filename, length), read_y_array(filename, length)) in the Python_Simplified version of readTable.py have been introduced to replace Python's version (which only contained read_table(filename)).

  • MAINFUN IMPLEMENTATION differences between Python and Python_Simplified versions : https://www.diffchecker.com/U73K51SG

  • MAINFUN TESTING is improved in /Test since it has all the necessary files for input and output (which were the only discrepancies between the testMain{#}.py files in /OldTests) in main.txt; seems to be breaking in relation to interp

@niazim3

This comment has been minimized.

@smiths
Copy link
Owner

smiths commented May 30, 2018

@niazim3, please feel free to improve the test code and documentation as you see fit. The eventual goal is to write something that you wish you had found when you first looked at the test code. 😄 In writing the new test documentation and test code, please change the current names to more meaningful names. testTable1 and testTable2 do not tell us anything. It is also good if the test case summaries do not needlessly repeat information. If many test cases share inputs, then we should only summarize these inputs once, and simply distinguish each test case by how it is different from the others. I made a similar comment for:

JacquesCarette/Drasil#330

Perhaps you and @elwazana should discuss the best way to document the test cases? Our goal is something that makes sense to humans, but that we can also imagine generating from the knowledge in Drasil.

@niazim3
Copy link
Collaborator Author

niazim3 commented Jun 5, 2018

As of commit 01e1337, testReadTable is no longer causing failures or errors. However, the test definitions use numbers that seem to come out of nowhere when looking at just the code. Either

  • upon documenting the tests we'll be able to determine why these particular numbers are used, or
  • read_x_array or read_y_array will be updated to use the length input parameters to produce these number lists (will still need to determine semantics of such a list)

For now, just going to point out that these numbers are the first length number of items in the lists produced of the first length+1 lines that are returned from the functions. E.g. for

def test_read_x_array(self):
self.assertTrue((self.array1[0] == np.array([[6.143397364345, 6.128184215473, 6.344639409554, 7.199850837298],
[6.158648279689, 6.158648279689, 6.344639409554, 7.21772438659],
[6.189263785047, 6.189263785047, 6.376179502933, 7.253604707984],
[6.189263785047, 6.189263785047, 6.407876386546, 7.271611700659],
[6.204628563268, 6.22003148438, 6.407876386546, 7.289663395493]])).all())
, where the read_x_array('TSD.txt', len(w_array)) call produced the following list (partial list of the approximately 500 lines produced):
image

@niazim3
Copy link
Collaborator Author

niazim3 commented Jun 7, 2018

Not sure why these weren't coming up earlier, but printing (self.errorMsg[i]) + " is " + context.exception.args[0]) in test_check_constraints leads to the following code snippet. We need to look through the input files to ensure different types of invalid inputs are being entered (which can be done systematically after test cases are documented following #80).

InputError: a and b must be greater than 0 is InputError: a and b must be greater than 0
InputError: a and b must be greater than 0 is InputError: a and b must be greater than 0
InputError: a/b must be between 1 and 5 is InputError: a/b must be between 1 and 5
InputError: a/b must be between 1 and 5 is InputError: a/b must be between 1 and 5
InputError: t must be in [2.5,2.7,3.0,4.0,5.0,6.0,8.0,10.0,12.0,16.0,19.0,22.0] is InputError: a/b must be between 1 and 5
InputError: wtnt must be between 4.5 and 910 is InputError: a/b must be between 1 and 5
InputError: wtnt must be between 4.5 and 910 is InputError: a/b must be between 1 and 5
InputError: TNT must be greater than 0 is InputError: a/b must be between 1 and 5
InputError: SD must be between 6 and 130 is InputError: a/b must be between 1 and 5
InputError: SD must be between 6 and 130 is InputError: a/b must be between 1 and 5
InputError: a and b must be greater than 0 is InputError: a and b must be greater than 0
InputError: a and b must be greater than 0 is InputError: a and b must be greater than 0
InputError: TNT must be greater than 0 is InputError: a/b must be between 1 and 5

.

@niazim3
Copy link
Collaborator Author

niazim3 commented Jul 26, 2018

@smiths Should this issue be closed since #134 is tracking the new code for GlassBR and new tests will be implemented for that implementation?

@smiths
Copy link
Owner

smiths commented Jul 26, 2018

Yes, I think we should close this issue. Part of my motivation for wanting to redesign GlassBR was the confusion around testing in the previous version. I think things should fit together better with the redesign.

@niazim3 niazim3 closed this as completed Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants