-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement GlassBR design in Python code #134
Comments
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
To Do:Just noting here that as mentioned in the following comment ( 03fdd1b#commitcomment-29848126):
|
Sounds good. |
@smiths Hello Dr. Smith, I just had a question about the old implementation default values, did those values pass? |
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". |
@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: 81716bc |
@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. |
@smiths, the data constraints for a and b are given by the SRS as: The values of t (nominal thickness) are given as: 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: 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? |
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? |
@smiths, I made the following changes to the Data Constraints and Auxillary constants: As for the requirement, R3: Here is the commit with the changes: |
@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. 😄 |
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. |
As mentioned in #105; this issue is just to keep track of the progress made for the task.
Hardware ModuleNote: be sure to use doxygen-style comments for the new implementation
The text was updated successfully, but these errors were encountered: