Conversion factor in calc_J_tol()

[elwazana]

@smiths Hello Dr. Smith, I believe that these need to be changed to not have a conversion factor as we discussed in https://github.com/JacquesCarette/Drasil/issues/824#issuecomment-404516544:

I've also noticed that there seems to be an extra = here which isn't present in the other calc* functions.

[smiths]

@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.

[elwazana]

@smiths Hello Dr. Smith, I've corrected the error here: ad21b0f9edf1bd18e74404b6e9053a018a2c9a50, and these are the produced changes:

[smiths]

Great! One extra small change to make is to remove the superfluous brackets. Eh^2 will look better than (E) (h)^2. 😄

[elwazana]

@smiths These are the changes and commit: 0f0b29834b2c9cbb5b4bb7f6b448430f4c11f5b4

[smiths]

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.

[elwazana]

@smiths I've fixed that problem here: ffc015d42b83467973afc9adb97b20100228dabf

[smiths]

You missed a and b.

[elwazana]

@smiths Sorry about that had a brain fart, this is the correct full change: c2adfb429ac62f5be5a3da82f92589cd290d54b9

[smiths]

Great!

[elwazana]

@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?

[smiths]

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.

[elwazana]

@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, E was set as 7.17e7 but that's Es value in kPa not Pa. This led me to find that the old implementation was wrong;

Old MIS: j_tol takes E was Pa, that's why the original value of 7.17e7 kPa is multiplied by 1000 to get 7.17e10 Pa which is what was used here:

Old MIS: But q_hat takes E as kPa, which is why it doesn't multiply it by 1000, it keeps it at 7.17e7 kPa:

When these two equations were copied over to the current MIS, this inconsistency wasn't accounted for. Meaning, j_tol now expects E as Pa which is good this allowed us to remove the conversion factor.

But q_hat's inconsistency was never accounted so its still expecting E as kPa, not Pa. That's is why as I mentioned above, fixing one breaks the other, because they expect different units for the same input.

[smiths]

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.

[samm82]

When referring to issue 130 on JacquesCarette/Drasil#824, I meant this issue (oops). I think this issue can be closed @smiths

[smiths]

Yes, we can close this issue. Thank you @samm82.