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

Refactor LBT09 layout to work with trim_levels_to_map #110

Closed
5 tasks done
anajens opened this issue Aug 27, 2021 · 5 comments · Fixed by #153
Closed
5 tasks done

Refactor LBT09 layout to work with trim_levels_to_map #110

anajens opened this issue Aug 27, 2021 · 5 comments · Fixed by #153

Comments

@anajens
Copy link
Contributor

anajens commented Aug 27, 2021

Redesign family of functions used to create LBT09 layout to work with rtables::trim_levels_to_map. Background issue (insightsengineering/rtables#203)

Rationale: improved speed, allow users to precisely control which combinations of levels among several categorical variables used in the layout should be displayed.

Note note all of the step above may be relevant for the layout. If an update is necessary, please add the PR link below.

  • tern design doc: 110_LBT09_design_doc_new_split_fun #153
  • tern production code for s_, a_, layout function. Also please update relevant unit tests.
  • teal.modules.clinical update. Note: Modules should aways split on the empirical map (observed combinations in the data) to make processing fast.
  • TLG-C update
  • nsdl.tests update to compare with STREAM
@wwojciech wwojciech self-assigned this Aug 27, 2021
@anajens anajens added the blocked label Sep 8, 2021
@anajens
Copy link
Contributor Author

anajens commented Sep 10, 2021

@wwojciech take a look at the design doc for LBT04 in tern as the examples there should be relevant for this function too.

Also unblocking and moving to sprint 29 as g_lineplot work is mostly done now.

@anajens anajens removed the blocked label Sep 10, 2021
@wwojciech wwojciech added sme SP8 and removed SP8 labels Sep 13, 2021
@anajens
Copy link
Contributor Author

anajens commented Sep 23, 2021

Not ready for October release so moving to SME backlog.

@wwojciech wwojciech linked a pull request Sep 23, 2021 that will close this issue
@wwojciech
Copy link
Contributor

wwojciech commented Sep 23, 2021

Hi @anajens, PR (with design doc update) is ready for this issue. To simply the split map, I created new artificial indictor variable ALTAST_ind which will be used in the layout instead of PARAMCD. It seems that update of s_ and a_ is not really needed in this case. As the change to TLG-C is very subtle, let me know please if you would like to update the TLG-C LBT09 static entry in tlg-catalog to use the new layout instead of the old one.

@anajens
Copy link
Contributor Author

anajens commented Sep 28, 2021

Hi @wwojciech , I don't think I will have time to review this during the release sprint so I've moved your PR to the next sprint board.

@anajens
Copy link
Contributor Author

anajens commented Oct 13, 2021

@wwojciech just a heads up I added the nsdl.tests checkbox to the task list above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants