-
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
Conversion factor in calc_J_tol() #130
Comments
@elwazana, you are correct. The MIS was written by copying the SRS content at the time. The conversion factor should be changed to match the current version of the MIS. I must have made a mistake with the extra equal sign. Please remove it. |
Great! One extra small change to make is to remove the superfluous brackets. Eh^2 will look better than (E) (h)^2. 😄 |
Thank you for removing the brackets. You have made the variables text in the equation. Please revert them back to the way they were originally displayed. In mathematical equations the variables are displayed in italic font. That means that E, h, a and b should be in italic font. This is the default in LaTeX equations, so all you have to do is remove the \text that you added. You probably were confused by the use of \text{LDF}. This is confusing and not entirely consistent. You should leave LDF as \text to be consistent with everywhere else in the document. Plain LDF (without adornment) in LaTeX would be in italic, but spaced out as L times D times F, with the multiplication implied by proximity. This doesn't look right. It may have been a better choice to use \mathit{LDF}, in the first place, but I don't think we should waste time with that now. |
You missed a and b. |
Great! |
@smiths Hello Dr. Smith, I've been working on the Control Module and I've hit a snag while trying to test it. The snag is that q_hat and j_tol can be kept in bound, originally with the input values I was using q_hat was too large and going past what can be accepted. I was able to correct this by increasing h. But this led to j_tol being too large to be accepted. I was able to fix this and have the control module run fully by appending the conversion factor back into j_tol: I've been tracing back this issue to see why this may be and that's when I found this: It seems like the equation for j_tol and q_hat was inconsistent with the mm to m conversion factor, and when they were copied over to the new MIS only j_tol was corrected with issue #130. The Control module only seems to work if those conversion factors are reintroduced, should I create a separate issue for discussing this problem? |
Do not reintroduce the conversion factors. Make sure that your inputs are in m, not mm. As long as everything is consistent, it should work. Are all the other modules passing their unit tests? Do the unit tests make sense? The inputs for the tests should be using m, not mm. |
@smiths I think I found the problem the original implementation of the MIS was inconsistent with the units of the inputs. Here is the old implementation of the equations for calculating q_hat and j_tol. You were correct one of the inputs was incorrect, Old MIS: Old MIS: When these two equations were copied over to the current MIS, this inconsistency wasn't accounted for. Meaning, j_tol now expects But q_hat's inconsistency was never accounted so its still expecting |
Great catch @elwazana. The previous version of GlassBR was switched from kPa to Pa in an ad-hoc way. We weren't as careful as we should have been. That is part why I wanted to redo it, and why I wanted to emphasize testing this time. Our current approach has been much more rigorous, which has facilitated finding this error. Please make sure that the new code, and all of the documentation, reflects that E is in Pa. |
When referring to issue 130 on JacquesCarette/Drasil#824, I meant this issue (oops). I think this issue can be closed @smiths |
Yes, we can close this issue. Thank you @samm82. |
@smiths Hello Dr. Smith, I believe that these need to be changed to not have a conversion factor as we discussed in JacquesCarette/Drasil#824 (comment):
I've also noticed that there seems to be an extra
=
here which isn't present in the othercalc*
functions.The text was updated successfully, but these errors were encountered: