Skip to content

Introduce teal_data class - #171

Closed
gogonzo wants to merge 227 commits into
gh-pagesfrom
teal_data@main
Closed

Introduce teal_data class#171
gogonzo wants to merge 227 commits into
gh-pagesfrom
teal_data@main

Conversation

@gogonzo

@gogonzo gogonzo commented Oct 4, 2023

Copy link
Copy Markdown
Contributor

Part of insightsengineering/teal#937
Other PRs insightsengineering/teal#925 insightsengineering/teal.slice#440 insightsengineering/teal.gallery#75 insightsengineering/teal.code#153

install

staged.dependencies::dependency_table(
  project = "insightsengineering/teal@https://github.com",
  project_type = "repo@host",
  ref = "teal_data@main",
  verbose = 1
) |> staged.dependencies::install_deps()

teal_data class

tdata-qenv, tdata inheritance

  • teal_data class (teal_data-class.R) is en extension of qenv. teal_data contains extra slots datanames and join_keys. Besides, all teal.code methods works with teal_data object (eval_code, print etc.)
  • Old TealData$server returns teal_data now - to have single object type in teal (not in teal_module)
  • cdisc_data() and teal_data() return TealData or teal_data
ADSL <- synthetic_cdisc_data("latest")$adsl
ADTTE <- synthetic_cdisc_data("latest")$adtte
ADRS <- synthetic_cdisc_data("latest")$adrs

data <- cdisc_data(
  ADSL = ADSL,
  ADTTE = ADTTE,
  ADRS = ADRS,
  code = quote({
    ADSL <- synthetic_cdisc_data("latest")$adsl
    ADTTE <- synthetic_cdisc_data("latest")$adtte
    ADRS <- synthetic_cdisc_data("latest")$adrs
  })  
)

data |> eval_code(quote(a <- ADSL))

insights-engineering-bot and others added 30 commits March 2, 2022 13:08
Co-authored-by: dinakar29 <26552821+dinakar29@users.noreply.github.com>
* the code related to `teal`'s data models moved from the original teal and teal.devel packages

Related to insightsengineering/coredev-tasks#130
* fix vignette
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
* refactored `is` to `inherits`
* did some styling changes - someone really loved new lines when writing
  these tests

 Closes #11
* is to inherits
* update staged.deps

* add tmh to downstream

* Apply suggestions from code review

* to_relational_data.TealData - to return TealData

* Update R/to_relational_data.R

Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
* removed var_relabel

* removed variable_labels and updated staged.dependencies

* importFrom formatable var_labels

* docs

* removed extra space
* rename formatable into formatters

* actions
* NEWS

* Update NEWS.md
fixing preprocessing-data vignette
waiting for parser
preparing to use parser
teal_data no longer returns TealData when data are available.
fix
wrong package namespace for get_code
fixing R cmd check
@github-actions

github-actions Bot commented Oct 5, 2023

Copy link
Copy Markdown
Contributor

Unit Tests Summary

       1 files       27 suites   19s ⏱️
   365 tests    363 ✔️ 0 💤 0  2 🔥
1 011 runs  1 009 ✔️ 0 💤 0  2 🔥

For more details on these errors, see this check.

Results for commit 7cb1986.

♻️ This comment has been updated with latest results.

Comment thread DESCRIPTION Outdated
Comment thread R/mutate_dataset.R Outdated
Comment thread R/mutate_dataset.R Outdated
Comment thread R/teal_data-class.R Outdated
#' @name new_teal_data
#'
#' @param code (`character(1)` or `language`) code to evaluate. Accepts and stores comments also.
#' @param env (`list`) List of data.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unless I am mistaken, we intend to always pass a list of datasets to this argument. The argument name is misleading then. Yes, the datasets will be list2env'ed and end up in the @env slot but it is not an environment that we pass. Can we name this more appropriately?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True, at this moment we have:

new_qenv(env = "environment", code = c("character", "language"))
new_teal_data(env = "list", code = c("character", "language"))
teal/cdisc_data(..., code = c("character", "language"))

According to our discussion two days ago, we plan to remove possibility to add env and code in new_qenv (we need to soft deprecate this). In the new_teal_data, i would change argument env to data (a list).

new_qenv()
new_teal_data(env = "??", code = c("character", "language"))
teal/cdisc_data(..., code = c("character", "language"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds great 👍

Comment thread R/teal_data-class.R Outdated
new_teal_data(env) new_teal_data(data)
fixing namespace warning
Comment thread DESCRIPTION
Comment thread DESCRIPTION

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

vbump to 0.4.0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What is the convention on @main to signal minor version bump? Should I set version from 0.3.0.9xxx 0.3.9.9xxx?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Set it to the target 0.4.0 value and merge the whole PR with [skip vbump]
@cicdguy please confirm

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct. Bump to 0.4.0 for minor version.

The first digit is for major breaking changes.
The second for minor changes like features which do not break the core API.
The third digit is for patches and bugfixes.

So unless you're making a new release out of this, I'd say the new version is just a development version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we do it now or shall we wait for the pre-release clean-ups? I think the latter as there might be other PRs merged in afterwards which will make it 0.4.0.9xxx and from there it would be problematic to "downgrade" into the target 0.4.0. So essentially we can address it during pre-release clean-up. Is that right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes I would prefer doing it here instead of doing it later and downgrading it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I wasn't clear. It is either:

  • do it now -> expect upcoming vbumps during standard dev cycle or hot-fixes -> downgrade during pre-release clean-ups
  • wait and do it later during pre-release clean-ups

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just clarified this - yes let's wait and do it later during pre-release cleanups (aka when the PR is being prepared for an upcoming release).

@donyunardi

Copy link
Copy Markdown
Contributor

Please hold on merging this to main branch.
See discussion here:
insightsengineering/nestdevs-tasks#41

pawelru and others added 5 commits October 16, 2023 13:02
- Config/Needs/verdepcheck entries in new line
- add `bioc::` for packages from Bioconductor

Signed-off-by: Pawel Rucki <12943682+pawelru@users.noreply.github.com>
need higher version of teal.code - code slot is a character
update pkgdown
@gogonzo
gogonzo changed the base branch from main to gh-pages October 18, 2023 12:27
@gogonzo gogonzo closed this Oct 18, 2023
@gogonzo

gogonzo commented Oct 18, 2023

Copy link
Copy Markdown
Contributor Author

By accident I've closed this PR without a change to open again. New PR here #178

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.