barrettk
commented
2 years ago Waiting to merge this PR until the following PR's have been merged to main, and pulled into here (expecting conflicts):
Closed barrettk closed 2 years ago
barrettk
commented
2 years ago Waiting to merge this PR until the following PR's have been merged to main, and pulled into here (expecting conflicts):
barrettk
commented
2 years ago Apparently none of the ROT-R requirements are part of a story (15 in total). These requirements are a bit all over the place, but primarily deal with check_grd() and other check_ functions. Given that these are backend functions, im wondering if we should have requirements for them or not. If we do want a new story, would definitely like some advice on what the description/name should be.
For reference:
ROT-R001:
description: check_file returns correctly
tests:
- BBR-ROT-001
ROT-R002:
description: check_file supports head and tail arguments
tests:
- BBR-ROT-002
ROT-R003:
description: tail_output() works
tests:
- BBR-ROT-003
ROT-R004:
description: tail_lst() works
tests:
- BBR-ROT-004
ROT-R005:
description: check_output_dir() works
tests:
- BBR-ROT-005
ROT-R006:
description: check_output_dir() works with filter
tests:
- BBR-ROT-006
ROT-R007:
description: check_nonmem_table_output() output matches ref df
tests:
- BBR-ROT-007
ROT-R008:
description: check_nonmem_table_output(.x_floor=0) works
tests:
- BBR-ROT-008
ROT-R009:
description: check_ext() works with character directory and model object
tests:
- BBR-ROT-009
ROT-R010:
description: check_ext() works with summary object
tests:
- BBR-ROT-010
ROT-R011:
description: check_ext() works when .iter_floor argument is NULL
tests:
- BBR-ROT-011
ROT-R012:
description: check_grd() works with character directory and model object
tests:
- BBR-ROT-012
ROT-R013:
description: check_grd() works with summary object
tests:
- BBR-ROT-013
ROT-R014:
description: check_grd() works when .iter_floor argument is non-zero integer
tests:
- BBR-ROT-014
ROT-R015:
description: check_grd() when .iter_floor argument is NULL
tests:
- BBR-ROT-015
kyleam
commented
2 years ago Apparently none of the
ROT-Rrequirements are part of a story (15 in total)
The ones that go through check_nonmem_table_output are deprecated, so I don't think those are worth adding.
check_file-based ones and check_output_dir are not deprecated and are exported for the end user, so I'd say those are worth adding a story for. Perhaps something like
diff --git a/inst/validation/bbr-stories.yaml b/inst/validation/bbr-stories.yaml
index 7b6e0cf9..e0c6815f 100644
--- a/inst/validation/bbr-stories.yaml
+++ b/inst/validation/bbr-stories.yaml
@@ -379,6 +379,18 @@ OUT-S003:
- NMJ-R001
- NMJ-R007
- NMF-R008
+OUT-S004:
+ name: Inspect model output files
+ description: As a user, I want to be able pass a model object to
+ list files in the output directory and to inspect the beginning and
+ end of key output files.
+ requirements:
+ - ROT-R001
+ - ROT-R002
+ - ROT-R003
+ - ROT-R004
+ - ROT-R005
+ - ROT-R006
LOG-S001:
name: Parse model YAML files into run log table
description: As a user, I want parse all model YAML files in a directory (and optionallyApparently none of the
ROT-Rrequirements are part of a story (15 in total)The ones that go through
check_nonmem_table_outputare deprecated, so I don't think those are worth adding.
check_file-based ones andcheck_output_dirare not deprecated and are exported for the end user, so I'd say those are worth adding a story for. Perhaps something likediff --git a/inst/validation/bbr-stories.yaml b/inst/validation/bbr-stories.yaml index 7b6e0cf9..e0c6815f 100644 --- a/inst/validation/bbr-stories.yaml +++ b/inst/validation/bbr-stories.yaml @@ -379,6 +379,18 @@ OUT-S003: - NMJ-R001 - NMJ-R007 - NMF-R008 +OUT-S004: + name: Inspect files output files + description: As a user, I want to be able pass a model object to + list files in the output directory and to inspect the beginning and + end of key output files. + requirements: + - ROT-R001 + - ROT-R002 + - ROT-R003 + - ROT-R004 + - ROT-R005 + - ROT-R006 LOG-S001: name: Parse model YAML files into run log table description: As a user, I want parse all model YAML files in a directory (and optionally
Yeah I think that makes sense. I like that description too
barrettk
commented
2 years ago @seth127 @kyleam Given the large number of changes to requirements/stories I figured you both want to take a look at this (even if only one of you reviews it)
kyleam
commented
2 years ago Here are the relevant PRs in this release that didn't touch inst/validation/. The ones marked with * were added by this PR.
* dcc58350 Merge pull request #480 from metrumresearchgroup/feat/wait_for_nonmem
* 1149a981 Merge pull request #484 from metrumresearchgroup/patch/filter-log
5b57feed Merge pull request #489 from metrumresearchgroup/patch/test-threads
* 85886890 Merge pull request #487 from metrumresearchgroup/patch/starred-models
2e407cc7 Merge pull request #499 from metrumresearchgroup/patch/use_bbi
f49748b8 Merge pull request #496 from metrumresearchgroup/patch/test-threads-error
0032b76b Merge pull request #508 from metrumresearchgroup/fix/nm_join
03389943 Merge pull request #519 from metrumresearchgroup/patch/test-threads
14779e35 Merge pull request #524 from metrumresearchgroup/patch/bbi_null_num
0df5954c Merge pull request #539 from metrumresearchgroup/patch/row-printNone of the remaining ones seems like ones that need to be added here (and some are just follow-up PRs on previous ones), though I could see some of them (e.g., 499 or 524) going either way.
So, what's included here looks fine to me, and, reading through the diff, the fixes and additions look good.
The one thing I'm curious about is when you're choosing to re-level the numbers (e.g., NMJ-R006 and friends after removal of NMJ-R005) and to not (e.g., other PLB-RNNNs weren't adjusted with removal of PLB-R001). Is there a particular logic/rule that's being applied?
barrettk
commented
2 years ago The one thing I'm curious about is when you're choosing to re-level the numbers (e.g.,
NMJ-R006and friends after removal ofNMJ-R005) and to not (e.g., otherPLB-RNNNs weren't adjusted with removal ofPLB-R001). Is there a particular logic/rule that's being applied?
I only decided against it because it would have been more involved, and I was nervous even doing the other one. I suppose thats the purpose of a review though. I can adjust those as well though.
FYI, I now think I should first merge this PR, and then pull these changes into the test-threads PR. Im touching a few requirements, and it would be much easier to stack them on top of these changes.
kyleam
commented
2 years ago I only decided against it because it would have been more involved [...]
I'm unsure in general what the approach should be for re-leveling IDs. Given part of the point in having IDs is stability, I guess we don't want to be regularly shifting them, though that would mean having gaps that may raise eyebrows when the docs reviewed. (Any thoughts, @seth127?)
But for requirement IDs this round, I think any amount of re-numbering is okay because the 1.3.1 didn't have requirements yet, only stories and test IDs.
barrettk
commented
2 years ago Im not sure why I was nervous before, but I think it was because I thought I would have to change the tests as well (which I obviously dont). It was an easy fix (just committed).
I'm unsure in general what the approach should be for re-leveling IDs. Given part of the point in having IDs is stability, I guess we don't want to be regularly shifting them, though that would mean having gaps that may raise eyebrows when the docs reviewed. (Any thoughts, @seth127?)
But yeah exactly. Its good we got rid of a lot of requirements we dont need this round, but unsure what the protocol would be in the future. I would think not touching them would be the safest, but it does bother me when I see skips because it looks like some are missing. I feel like an auditor would have the same concern.
Would be nice if we had a function that could do this shifting, and update the stories to reflect it, but that might be overkill
seth127
commented
2 years ago In terms of re-levelling IDs, I agree that it doesn't really matter for this PR because we didn't have reqs on 1.3.1.
That said, going forward I think we should probably not change IDs unless there's a compelling reason to do it. Reasons:
Some of the early PR's were missing requirements/stories, as we weren't in the habit of adding them per PR early on for this milestone. In addition, some tests/requirements were renamed/moved
closes https://github.com/metrumresearchgroup/bbr/issues/502