-
-
Notifications
You must be signed in to change notification settings - Fork 1
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 AET01_AESI #348
Add AET01_AESI #348
Conversation
🧪 Code Coverage Summary
Results for commit: f85a2e235eca9a30ac712cd1f60983f0df2f4379 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
@clarkliming @barnett11 A general comment - The optional lines (specified in aesi_vars) are displayed as default, which isn't the standard display for this output. Should we remove these optional lines and give the user option to add them in as needed? Rather than displaying them all as default? |
if by default we don't have these optional lines, then we should make it the default one, instead of having other lines. However, I still thinks that the structures of "total number of xxx" and "No. of " is difficult to handle here. Since we are using labels (with spaces), I think we should, include all the summary vars in the argument, including ALL_RESOLVED and NOT_RESOLVED, SER and REL. What do you think @edelarua and @barnett11 |
@clarkliming I think it makes sense logically to include ALL_RESOLVED/NOT_RESOLVED/SER/REL as optional vars in the argument, but an issue arises if the user chooses to include indented sub-variables without also including the related "total" var, as this would make the resulting structure of the output table quite confusing for the user. E.g.
|
Hi @clarkliming , @edelarua , |
There was a problem hiding this 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; the preprocessing and in main the aesi_vars part are complicated but we can refine later;
Closes insightsengineering/chevron-tasks#36