smiths / caseStudies

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

Review of SRS data constraints vs Input Module exceptions #129

Closed niazim3 closed 6 years ago

niazim3 commented 6 years ago

I compared the input data constraints table in the SRSs against the Input.verify_params() documentation image and noticed the following discrepancies:

1. (Within Drasil) The physical constraint for a : a <= b conflicts with that of b: 0 < b < a. Can a = b or not? If a cannot be = to b, a new error should be defined and an issue should be opened in the Drasil repo about making the change, otherwise, MIS list is fine.

image

2. Should AR be added as an input data constraint in Drasil since there is an error message associated with incorrectly input AR?

3. Is AR >= 0 (MIS) or AR >= 1 (manual SRS)?

(Below is the table for data constraints from the manual version for reference image.)

smiths commented 6 years ago

The constraints for w are discussed in: https://github.com/JacquesCarette/Drasil/issues/924 The constraint for AR should be >= 1.0 (not 0).

The constraints in Drasil related to a and b are confusing. That is why the aspect ratio AR has been added to the constraints. It should be easier to understand with AR. a can be equal to b. That just means a square plate. That error was introduced into Drasil at some point.

Yes, AR should be added to Drasil.

elwazana commented 6 years ago

@smiths Hello Dr. Smith is there anything else that should be added to this checklist:

elwazana commented 6 years ago

This is the b constraints fix in the manual SRS: 2b1252b09fb72e3c8b3611268949e68a02ae2782 image

smiths commented 6 years ago

Looks good to me. I believe there is another issue for this, but the mm units also need to be changed to m.

elwazana commented 6 years ago

This is the AR constraint fix in the MIS: 256521572c504c481b407ddd72293bddcdca3d37 image

@smiths I've also highlighted this second occurrence of AR, which I noticed was also highlighted above. Does it need to be modified in any way?

smiths commented 6 years ago

The second occurrence of AR is fine.

elwazana commented 6 years ago

@smiths I've begun working on the Drasil side for this issue when I found this: image

Does this need to be updated as above where a can be equal to b?

Also, I was able to correct the a can equal b in the Data Constraints: image

smiths commented 6 years ago

Yes, in the definition of AR, a >= b.

elwazana commented 6 years ago

This is the last fix for a and b: image

I've started to propagate the AR constraints into the Data Constraints for Drasil: image

@smiths Where do I retrieve the Typical Value and Uncert. for AR from?

Additionally none of the listed Typical Values or Uncert. really state where they are retrieved from which seems like a traceability issue (Both Drasil and Manual).

Also should AR have its own row in the Data Constraint table? In the manual its weaved into the rows of a and b: Manual: image

This would be difficult to do in Drasil because Drasil can't have the constraints of two different symbols in the same row.

smiths commented 6 years ago

Great questions. For the typical value of AR, I suggest 1.5. For uncertainty, use 10%. It would be great to have traceability to the typical values and uncertainty, but we don't really have a source for this information. The uncertainty in particular is rarely discussed. The SRS is intended to get people to think about uncertainty going forward, but it isn't standard procedure today.

I like the idea of AR having its own row. As you said, it helps Drasil and it is also cleaner in the manual version. Please create a new issue for this.

elwazana commented 6 years ago

@smiths Hello Dr. Smith, here is the final table with the modifications to b and AR completed: https://github.com/JacquesCarette/Drasil/commit/d3275fc06364b2177c4a17c1e273aa8fe8bc2aad image