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

Differences in Table of Symbols in GlassBR SRS #824

Closed
7 tasks done
samm82 opened this issue Jul 11, 2018 · 29 comments
Closed
7 tasks done

Differences in Table of Symbols in GlassBR SRS #824

samm82 opened this issue Jul 11, 2018 · 29 comments
Assignees

Comments

@samm82
Copy link
Collaborator

samm82 commented Jul 11, 2018

The following need to be fixed in caseStudies:

  • Fix no line breaks in caseStudies
    • Note: "My first though is to use the p{width} specifier in the LaTeX listing of column justification. You could also look at the generated LaTeX to see how it is done there."
  • Define SDx, SDy, and SDz explicitly in caseStudies
  • Define the following in caseStudies: ARmax, AR, GTF, LDF, LR, LSF, NFL, TNT
  • Change units for a, b, and h to meters in caseStudies
  • Change "second" to "s" in caseStudies
  • Swap "B" and "b" in caseStudies
  • Change "Explosive Mass..." to "Explosive mass..." in caseStudies

table of symbols

@smiths

This comment has been minimized.

@elwazana
Copy link
Collaborator

@smiths Hello Dr. Smith, I just wanted to ask you if the solution is ok for the third task in the checklist:

When adding the definitions for ARmax, AR, GTF, LDF, LR, LSF, NFL, TNT. The table would fall behind the next pages back:
image

I can fix it, but it requires me to hardcode it to be two separate tables:
image

@smiths
Copy link
Collaborator

smiths commented Jul 11, 2018

We don't want to hard-code two tables. This sounds like a LaTeX issue. longtable should appropriately split between two pages automatically. I don't know what tabular environment is being used, but look for one that automatically splits between tables.

Since you and @samm82 are both currently looking at GlassBR, please make sure to coordinate your efforts.

@elwazana
Copy link
Collaborator

This is the finished caseStudies Table fo Symbols:
image

@elwazana
Copy link
Collaborator

This is the commit with the changes:
smiths/caseStudies@f6fb98c

@smiths
Copy link
Collaborator

smiths commented Jul 11, 2018

Thank you for updating the table of symbols. Looks good. This edit highlights other changes to the case study SRS that need to be made.

In thinking about it further, we may have made a mistake in changing a and b to SI (m instead of mm). What units does the Drasil code use? (My stack is currently broken so I cannot easily generate the documentation). I would rather use SI standard units throughout, but this change has a cascading effect. Many equations will need to be changed and the MIS will need to be changed.

If the Drasil code for GlassBR is assuming mm for a, b, h etc, then we should stick with this decision. This will mean backing out the changes to a, b and h in the table of symbols. If the Drasil code already uses m, then we'll have to update the case study to use consistent units.

Currently q is in kPa in the case study. Is this the case in the Drasil code too? Eventually I would like it in Pa, but we shouldn't make this change when there are so many other changes going on at the same time.

Based on your feedback we can decide what to do with the units. The goal is to select the course that has the least amount of effect.

@elwazana
Copy link
Collaborator

@smiths Comparison report:

  1. a,b, and h have (m) listed as their units in Drasil.

  2. q has kPa listed as its units in Drasil.

  3. One difference I noticed is that E has different units, in the manual its kPa, but in Drasil its Pa.
    (Top Manual | Bottom Drasil)
    image
    image

  4. As a note, SI units aren't used for t, dmax, and dmin, which have (mm) listed as their units

@smiths
Copy link
Collaborator

smiths commented Jul 11, 2018

Thank you @elwazana. Are the units for a, b and h consistent throughout the document? Several equations, like the equation for B, depend on a, b and h. If the units for a, b and h are in mm, then the equation includes a conversion factor. If the units are in m, then the conversion factor isn't there. Hopefully Drasil is correct and a, b and h are not converted. I seem to recall we went through the Drasil version and made everything into the base units. That would also explain why E would be in Pa in the Drasil version. I'm surprised that q is in kPa though, since that would make those equations that involve q potentially inconsistent. It is possible that the original source material included the unit conversion.

@elwazana
Copy link
Collaborator

@smiths Hello Dr. Smith, a, b, and h seem to consistently appear as (m)
Drasil:
image

However, the manual seems to have a conversion factor from (mm -> m), which should be changed now that a, b, and h have been defined as being (m) in the table fo symbols.
There also seems to be a conversion factor for E from (kPa -> Pa), which if you decide to go with Pa as the defined unit will need to be changed where ever E is used in the Manual.
Manual:
image

For q, it seems to be consistently used as kPa with no conversion factor for both Drasil and the Manual.
Drasil:
image

Manual:
image

@smiths
Copy link
Collaborator

smiths commented Jul 12, 2018

Great summary @elwazana. Unfortunately this makes things more confusing and muddled. The conversion factor seems to be there when a and b are in mm and not there when a and b are in m. Having E in Pa and q in kPa is not consistent.

We need to sort this out once and for all. Can you please find the original equations in the source material, along with the units used for those equations. The source material should all be in the repo. We need to make sure that our choices are correct. Once we have this information, we can decide the best course of action. I still prefer everything in standard units: m, Pa, etc., but if this makes the equations too awkward, we might go another route.

@elwazana
Copy link
Collaborator

@smiths Just a quick question so I don't accidentally look over the wrong document, by source material do you mean in the src folder?

@smiths
Copy link
Collaborator

smiths commented Jul 12, 2018

Did you look in the src folder? 😄 The src folder is code. If there is a mistake in the requirement, it will be in the code too. The source information is given in the SRS in the Source field. You can find the documents that matter in the refs folder in the repo.

@elwazana
Copy link
Collaborator

@smiths Hello Dr. Smith, the original equations seem to be the ones in Drasil, although the parts I circled in red don't appear in the Drasil/Manual versions.

Drasil:

Original:
image

Drasil:

Original:
image

@smiths
Copy link
Collaborator

smiths commented Jul 12, 2018

Great investigation @elwazana. I was starting to doubt that we could sort this out and be sure to be consistent. So many changes are going on concurrently and some of the previous students (and myself) weren't as careful as we should have been. With your information we can make sure that the Drasil version is correct for the units, and then propagate these corrections to the case study version.

For the dimensionless load (q hat), I see how we can fix things to make it correct and consistent. Please make the following modification to the Drasil version (and subsequently to the Case Study version):

  • q should be Pa (if q is in kPa and E is in Pa there will be an error. q is dimensionless so the units are supposed to cancel. If both q and E are in Pa, or both in kPa, it will work, but if the units are different we have an extra 1000 floating around. Since we want the units to be in the standard units, let us change everything to Pa
  • add a note (under Notes) that this formula is for a single lite (A5) so LSF = 1.0. This equation was simplified from the original source because LSF is assumed to be 1.0 in A5 (from the Case Study numbering, in case they are different). We didn't explain this well in the original document though, so you, and anyone else looking at the equation, are going to wonder what is happening. If we add traceability to the assumption, then the consequences of changing this assumption are clear. (Currently changing A5 does not explicitly show an impact on the data definition. Good catch!)
  • Under the Source field, please add a reference to the source that you used for the experts above. I'm sorry that the Source field was previously empty. This should have been easier for you. That is the entire point of the Source field! Now we have the chance to correct this mistake. Even better, please include the Equation number with the source. We should make this as easy as possible to see where the information came from.

We should leave the dimensions (a, b and h) in m. Technically it doesn't matter, as long as they are consistent, since the units will cancel out. However, for reasons we have discussed previously, I prefer m.

For the risk of failure, the Drasil version looks almost right. Please make the following changes:

  • Remove the factor of 1000 from the equation (in front of E). (The fact that this slipped through is frustrating. At least when Drasil is working properly changes like this will easily propagate through all of the artifacts. Too bad we have to do so much manually right now.)
  • For the Source [4] please add the equation number that you used. As I mentioned above, it is ideal if we can have full traceability. Not just to the document, but to the parts of the document that we are using.

As far the part circled in red, we shouldn't have to worry. The value of J is calculated in DD4 (Stress Distribution Factor) (Case Study Version).

You might want to create a new issue for these changes, so that it doesn't get too lost in the mix.

Thank you again for the investigation. I don't like that it was necessary, but I do like that it worked fairly smoothly. 😄

@elwazana
Copy link
Collaborator

@smiths Hello Dr. Smith, for the source field of the segments I retrieved those from:
image
The document is not in the references for GlassBR, and you can't reference that isn't defined somewhere in the example (the references already there are defined as Contents).
So how should I put it in the source field?

Here are the other two requested changes related to q:
image

@smiths
Copy link
Collaborator

smiths commented Jul 12, 2018

I hadn't realized that you used Manuel's document, but this isn't a problem. You can create a bib entry for it and then reference that. The better job we do with traceability to the actual source of the information, the more useful the documentation is. If you need an example of creating bibliographic information for a reference like this, you can follow the Marilyn Lightstone reference for SWHS.

The citation for the definition of q hat should be in the Source field, not in the Notes field.

If you have problems with adding new bib information, I'm pretty sure your colleagues can help you.

@elwazana
Copy link
Collaborator

@smiths Hello Dr. Smith, I've moved the citations for the definition of q hat and I've created and cited the appropriate Document reference:
image
image

With regards to the equation numbers, is there a format you have in mind for how they should appear in the source field?

@smiths
Copy link
Collaborator

smiths commented Jul 13, 2018

Great to see the source updated. What does [glassLite] mean? I would think you would just have the [5]? As far as referencing the equation, I would think you would have something like:

[5, Eq. 13]

I made up the equation number. You would actually want to look at the source to see the real number.

@samm82 samm82 mentioned this issue Jul 13, 2018
3 tasks
@elwazana
Copy link
Collaborator

@smiths Hello Dr. Smith, I was able to generate the following:
image
image

However, to do this I needed to revert a change the yuzhi branch had once it was merged into master earlier today. The change had a "FIXME" placed inside all the source fields in all the tables, my reversion now causes all the source fields to be empty or filled, i.e:
image

This is the commit from the yuzhi branch that I reversed: f70bb0a

@smiths
Copy link
Collaborator

smiths commented Jul 14, 2018

@elwazana, you should discuss this with @halonazhao and probably @niazim3. We might have to wait for the FIXME before updating this. You probably should back out your change.

Can you please make a new issue for adding this information to Drasil.

Also, please make an issue for doing this update in the case study version as well. You should be able to proceed with making this change in the case study version. For citations that have notes you can include the information in square brackets before the citations. (\cite[Eq.~7]{WhateverRef5Is})

@elwazana
Copy link
Collaborator

@smiths Hello Dr. Smith, it seems like the "FIXME" was just a placeholder and not referring to an existing problem that needs resolution, so should I go through with my changes?

@samm82

This comment has been minimized.

@szymczdm

This comment has been minimized.

@smiths

This comment has been minimized.

@elwazana
Copy link
Collaborator

@smiths Hello Dr. Smith, should I go through with the reintroduction of the source field for the Data Definitions? As I mentioned above the "Fixme" currently there is just a placeholder.

@JacquesCarette
Copy link
Owner

Yes, it makes sense to have an optional source field for Data Definitions.

@smiths
Copy link
Collaborator

smiths commented Jul 23, 2018

Agreed.

@samm82
Copy link
Collaborator Author

samm82 commented May 1, 2019

Not sure if this and #130 can be closed. It looks like they are based on the commit/PR history.

@smiths
Copy link
Collaborator

smiths commented May 1, 2019

Again, this issue is not one issue, but many issues combined. Even worse, the conversation shifts to other topics. We have to really try to stop doing this. 😄.

It seems this issue is about updating the case study (manual) version. I think we are fine to close this issue, since the changes seem to have been made, and further maintenance of the case studies is not a high priority at this time.

As far as #130, that issue should be discussed under that issue, not under this one. 😄

@smiths smiths closed this as completed May 1, 2019
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

5 participants