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

Add lbt07 #366

Merged
merged 11 commits into from
Feb 3, 2023
Merged

Add lbt07 #366

merged 11 commits into from
Feb 3, 2023

Conversation

ayogasekaram
Copy link
Contributor

@ayogasekaram ayogasekaram commented Jan 19, 2023

lbt07 template.

closes insightsengineering/chevron-tasks#27

@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2023

🧪 $Test coverage: 89.50%$

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ------------------------------------------------------------------------
R/aet01_aesi.R                 188       8  95.74%   39, 41-45, 272, 285
R/aet01.R                      330       6  98.18%   38, 240, 253, 316, 322, 554
R/aet02.R                      222       2  99.10%   136, 489
R/aet03.R                       85       0  100.00%
R/aet04.R                      104       6  94.23%   81-86
R/assertions.R                  36       0  100.00%
R/checks.R                      14       0  100.00%
R/chevron_tlg-S4class.R         21       0  100.00%
R/chevron_tlg-S4methods.R       42      16  61.90%   47-55, 90-98, 218-290
R/cmt01a.R                     191       0  100.00%
R/cmt02_pt.R                    52       0  100.00%
R/dmt01.R                       40       0  100.00%
R/dst01.R                      287       0  100.00%
R/dtht01.R                     104       0  100.00%
R/egt01.R                       46       0  100.00%
R/egt02.R                       60       0  100.00%
R/egt03.R                      129      85  34.11%   32-52, 111, 125-232, 291, 305-339
R/ext01.R                       81       8  90.12%   238-241, 245-248
R/gen_args.R                     1       1  0.00%    27
R/lbt01.R                       95       0  100.00%
R/lbt04.R                       52       1  98.08%   127
R/lbt05.R                       70       2  97.14%   121, 149
R/lbt07.R                       81       2  97.53%   34, 164
R/lbt14.R                      196      28  85.71%   57, 59, 107-110, 112-117, 147, 178, 265, 267, 315-318, 320-325, 355, 386
R/mht01.R                       72       2  97.22%   33-34
R/mng01.R                       93      12  87.10%   113, 117-120, 129-137, 182
R/pdt01.R                       61      38  37.70%   35-52, 108-150
R/pdt02.R                       68       0  100.00%
R/utils.R                      163     109  33.13%   70, 83-212
R/vst01.R                       48       0  100.00%
R/vst02.R                      102       3  97.06%   42, 120, 257
TOTAL                         3134     329  89.50%

Results for commit: ea0290e0026458621290aadce0c760463f03a486

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2023

Unit Tests Summary

    1 files    25 suites   1m 18s ⏱️
112 tests   77 ✔️ 35 💤 0
241 runs  152 ✔️ 89 💤 0

Results for commit 37274e5.

♻️ This comment has been updated with latest results.

@ayogasekaram ayogasekaram changed the title 132 lbt07@main Add lbt07 Jan 19, 2023
@clarkliming clarkliming self-assigned this Jan 20, 2023
R/lbt07.R Outdated
#'
#' @inheritParams gen_args
#' @param lbl_param (`character`) label of the `PARAM` variable.
#' @param lbl_gradedir (`character`) label of the `GRADE_DIR` variable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can cause confusion: what is GRADE_DIR variable? though we are going to have changes to the preprocessing part, it is necessary to add more information on the derived variables for now

Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ayogasekaram ,
I get the below error on my data, do we need to ensure we're converting PARAM to factor if it is required?
Error: Error applying analysis function (var - GRADE_ANL): Assertion on 'df[[variables$param]]' failed: Must be of type 'factor' (or 'NULL'), not 'character'.

@npaszty
Copy link

npaszty commented Jan 25, 2023

@ayogasekaram

chatted with Joe last week and thought it would be helpful for me to review some PRs. Looking at the chevron readme and following the directions to install staged dependencies etc., I get errors that I can address by using chevron:::<function_name> but I can't get around the following error. I'm running this in an R 4.1.3 container.

Error in force(.formats) :
object 'format_count_fraction_fixed_dp' not found

I assume I'm doing something wrong but only find one reference to this object and it's on the right hand side of an assignment statement.

Might need a walkthrough to get me started...

@shajoezhu
Copy link
Contributor

hi @npaszty , I was wondering if you are using the latest tern?

@npaszty
Copy link

npaszty commented Jan 26, 2023

@shajoezhu
yes, I verified the installed version and what's in the description file.
0.1.1.9031

I updated the ref = from main to this branch for staged.dependencies and after I run build for chevron, the version changes to 0.1.1.9028. so something definitely up there. looking further into this.

this works to produce the AET02 table
db <- syn_test_data()
run(aet02_1, db)

but this doesn't, see error below
run(lbt07_1, db)

similar initial errors and now another error once addressed previous in same way.

Error in h(simpleError(msg, call)) :
error in evaluating the argument 'obj' in selecting a method for function 'last_rowsplit': error in evaluating the argument 'obj' in selecting a method for function 'next_rpos': error in evaluating the argument 'lyt' in selecting a method for function '.add_row_summary': error in evaluating the argument 'obj' in selecting a method for function 'next_rpos': error in evaluating the argument 'obj' in selecting a method for function 'next_cpos': could not find function "basic_table_deco"

best to have walkthrough so I'm not wasting developer time. I can't imagine my environment is properly set up but it's been a while since I did this for NEST so must be missing something or maybe readme needs update. let's circle back next Tuesday at our checkin

Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preprcoessing has filtering that is not described in the specification - can the redundant parts be removed please (eg. ANL01FL=Y)

@edelarua
Copy link
Contributor

edelarua commented Feb 2, 2023

@ayogasekaram FYI the egt01_1 failure is known and is being fixed by Davide here so that error can be ignored.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

JUnit report for branch main doesn't exist on _junit_xml_reports branch yet.
Once this workflow runs on main branch, you'll see comparison of tests performance between main and 132_lbt07@main as a PR comment.

Copy link
Contributor

@clarkliming clarkliming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the below is doing anything, as the values for missing are just "" rather than <Missing>
filter(.data$ATOXGR != "<Missing>"

@clarkliming
Copy link
Contributor

@shajoezhu maybe you can just add a explicit_na for ATOXGR just like other templates so that the missing values will be explicit missing and then remove those rows? And after this is completed we just merge this PR; @barnett11 If there are further comments, we can create another issue as this PR is getting large

@barnett11 barnett11 self-requested a review February 3, 2023 13:46
@clarkliming clarkliming merged commit 0f1306f into main Feb 3, 2023
@clarkliming clarkliming deleted the 132_lbt07@main branch February 3, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants