smiths / caseStudies

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

Implement GlassBR design in Python code #134

Open niazim3 opened 6 years ago

niazim3 commented 6 years ago

As mentioned in #105; this issue is just to keep track of the progress made for the task.

Note: be sure to use doxygen-style comments for the new implementation

smiths commented 6 years ago

Please also implement unit tests for all modules. You should use pytest, as we used in 2ME3. We should have a make rule that runs the tests.

smiths commented 6 years ago

@samm82 suggested a project for this implementation. I started a project, but I think I only assigned one issue to it. You can determine among yourselves whether an issue with a checklist is the way to go, or whether you want to use the project interface.

samm82 commented 6 years ago

@smiths This issue, along with the other ones we have created regarding the MIS, are all in the project already

smiths commented 6 years ago

I guess I should have thought to look. 😄 Looks good.

niazim3 commented 6 years ago

To Do:

Just noting here that as mentioned in the following comment ( https://github.com/smiths/caseStudies/commit/03fdd1b0d597613250c017996c8acfbacd22aa3c#commitcomment-29848126):

smiths commented 6 years ago

Sounds good.

elwazana commented 6 years ago

@smiths Hello Dr. Smith, I just had a question about the old implementation default values, did those values pass?

Old Implmentation input values: image

smiths commented 6 years ago

You would have to run the code to see if the input values pass. At the end of last summer all of the tests were passing, but things may have changed since then. In particular, there seems to have been a gradual move to update from mm to m. This code may have been caught in being only partially switched over.

If you are asking about mm versus m. We want to just work with m now. The input file should be changed accordingly. The only exception I can think of is for the thickness of the glass. That mm value isn't actually a measurement, but a label for the type of glass. That should stay as "mm".

elwazana commented 6 years ago

@smiths Hello Dr. Smith, I pushed the Control module as is and it completed. I've been trying to get a test for it to work and I noticed that when I set t = 5.0 or less and a and b are at their upper limit (their max value is set as 5 currently) the calculated value of q_hat becomes too big and exceeds its bounds.

Should the constraints on a and b be variable depending on the value of t? Or is something wrong with the equation?

The Control module can be found here: 81716bcbdc7ea26d4c886f839650fbd390368a2e

smiths commented 6 years ago

@elwazana, can you be more specific? If a and b are 5.0, then the aspect ratio is 1.0. Is this what you mean, or do you mean the aspect ratio is 5.0? What is the input values for q_hat? What is the value of q_hat? What are the bounds for q_hat that you are referring to? Is this related to the bounds over which the interpolation is defined?

Do the calculations work for other values of inputs? There will definitely be cases where we do not have data defined for the interpolations. In these cases it is completely acceptable to inform the user that their data exceeds the allowed bounds.

elwazana commented 6 years ago

@smiths, the data constraints for a and b are given by the SRS as: image (Where dmin = 0.1 & dmax = 5)

The values of t (nominal thickness) are given as: image

Jtol is calculated as such:

def calc_J_tol( params ):
    upper1 = 1
    lower1 = 1 - params.Pbtol

    upper2 = (params.a * params.b) ** (params.m - 1)
    lower2 = params.k * ((params.E * (params.h ** 2)) ** (params.m)) * params.LDF

    return (log( (log(upper1 / lower1)) * (upper2 / lower2) ))

By the equation above when a and b are large and t is small, Jtol becomes greater than its bounds mentioned here in the SRS: image

In the Control module, when Jtol is feed in to calculate other values such as q_hat and q_hattol, an OutOfDomain error is raised.

This happens despite a, b, and t being within the constraints as seen above. Should this be the case?

smiths commented 6 years ago

That helps. Thank you @elwazana. There is always going to be a point where the bounds for J are exceeded. It actually makes sense that having a huge plate of glass that is very thin is a bad idea. A 5 m by 5 m plate of glass (16 feet by 16 feet!) that is 5 mm thick seems ridiculous, even though I have no expertise in this field.

The constraints on a, b are not particularly educated constraints. I believe that they originally came from me and my thought that a plate larger than 5 m by 5 m seems impossible/improbable. No greater insight was involved, and no exploration of the mathematics.

We could look at the math and find a relationship between a*b and t, but we would have to assume that E, m and k are constant. (I know that they currently are, but they might not be in the future.) We could do this mathematical exercise, but we get the same information by calculating J in the program and seeing that it lies outside the bounds. You have done the important part of the analysis and shown that the reason it is outside of the bounds is that the dimensions of the glass are inadequate.

My recommendation is that the bounds on J be added to the SRS information. (I don't think they are currently in there, except implicitly through the graph you have shown above.) When J is outside of the valid range of 1 to 32, the required behaviour should be an error message that says "The value calculated for the stress distribution factor (J) lies outside the acceptable range [1, 32], likely as a result of inadequate plate dimensions."

If we make this part of the software behaviour, then your test case will now past, because the behaviour would now be what we expect.

Can you please update the SRS with this new data constraint information and requirement?

elwazana commented 6 years ago

@smiths, I made the following changes to the Data Constraints and Auxillary constants: image

image image

As for the requirement, R3: image Covers all data constraints already so I didn't add my own unless you had something else in mind for the updating the requirement as you mentioned above?

Here is the commit with the changes: 9dd83f083ba124fa926ac5a84e871e5408f7b8a5

smiths commented 6 years ago

@elwazana, J is not an input, so you cannot handle it in the same way as a, b, etc. J is one of the outputs, so I suggest that you put it in Table 4 (Output Variables). Just as Pb has a constraint on the valid output values, so does J.

You are right that R3 covers the inputs, but we should add a requirement on what happens when the outputs are invalid. The wording for this new requirement would be similar to R3. Not including a requirement like this was a mistake in the original case study, since as it is now, nothing is said about what to do if Pb is out of range. Thank you for bringing up this issue. 😄

elwazana commented 6 years ago

@smiths, I corrected the mistake with the tables and created a new requirement as you mentioned: image

For the requirement should it end with "the calculations stop" or should the program say the glass isn't safe/fails? image

Commit with changes: 590869e3cc74b9c41144776910aa40f88a09d97e

smiths commented 6 years ago

I actually like the symbolic names Jmin and Jmax better than the hard-coded values. Jmin and Jmax should stay as auxiliary constants, and Table 4 should use these constants. I'll change the case study version of the SRS.