Create records from data frames #78
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rcannood
reviewed
Nov 14, 2024
rcannood
reviewed
Nov 14, 2024
rcannood
reviewed
Nov 14, 2024
Gives a warning if it's not available. Module object is stored in the instance for further use.
rcannood
added a commit
that referenced
this pull request
Nov 14, 2024
Co-authored-by: Luke Zappia <lazappi@users.noreply.github.com>
rcannood
reviewed
Nov 15, 2024
R/Instance.R
Outdated
Comment on lines
1
to
4
| create_instance <- function(instance_settings, is_default = FALSE, py_lamin = NULL) { | ||
| super <- NULL # satisfy linter | ||
|
|
||
| api <- InstanceAPI$new(instance_settings = instance_settings) |
There was a problem hiding this comment.
- Can we create
py_lamininside ofcreate_instancesimilar to howapiis also created here depending on the value ofis_default? - If we create it in this function, we could also use lazy loading instead of immediately creating it here.
There was a problem hiding this comment.
I moved it but I deliberately wanted to check when the instance is created so users knows ASAP if they can't do some things. We could change that if you want though.
R/Instance.R
Outdated
Comment on lines
269
to
271
| py_lamin = function() { | ||
| private$.py_lamin | ||
| } |
There was a problem hiding this comment.
api is a public function. maybe make py_lamin also a public function instead of an active field?
There was a problem hiding this comment.
Moved. I'm not sure it actually helps to store the imported module so we can could also load it as needed.
rcannood
reviewed
Nov 15, 2024
rcannood
added a commit
that referenced
this pull request
Nov 15, 2024
* Migrate code from #78 to separate branch Co-authored-by: Luke Zappia <lazappi@users.noreply.github.com> * update imports * add more data loaders, align file and function naming to python implementation * wip add tests * run styler * add entry to changelog * don't use expect_equal to compare anndatas * Update R/core_loaders.R Co-authored-by: Luke Zappia <lazappi@users.noreply.github.com> * suggestions from review * fix roxygen * apply suggestion from review * update documentation --------- Co-authored-by: Luke Zappia <lazappi@users.noreply.github.com>
lazappi
changed the title
Added to the README. I already did the development vignette. Let me know if there is anywhere else. |
This was referenced Nov 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When finished this PR should:
from_df()method to (Artifact) registries to create artifacts from data framesRecordclasses to avoid API calls (needed for temporary records)delete()method to recordsOther Todos:
Removed:
Allow loading CSV and TSV artifactsAllow loading Parquet artifacts