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 optional convert options for r->clj functions #105

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

(-> r-object
(r options)
(r->java options)
(java2clj/java->clj options)))
(def r->clj r->java->clj)

(defn r->java->native-clj [r-object] (-> r r-object r->java java2clj/java->native))
Copy link
Member Author

Choose a reason for hiding this comment

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

I did not change this function to take options.
Could not find any usage, neither sucessfully call it myself.

@behrica behrica requested a review from genmeblog May 12, 2024 12:56
src/clojisr/v1/r.clj Outdated Show resolved Hide resolved
src/clojisr/v1/impl/java_to_clj.clj Outdated Show resolved Hide resolved
(defn r->java->clj [r-object] (-> r-object r r->java java2clj/java->clj))
(defn r->java->clj [r-object & options]
(-> r-object
(r options)
Copy link
Member

Choose a reason for hiding this comment

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

Why r should know about options?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm. Not sure any more. Was some time ago.
I think the idea was to "be able" to pass options through all layers.

Copy link
Member

Choose a reason for hiding this comment

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

Options should be passed only to the java->clj function.

Multidimensional conversion is done here: https://github.com/scicloj/clojisr/blob/master/src/clojisr/v1/impl/java_to_clj.clj#L94-L100

@behrica
Copy link
Member Author

behrica commented Sep 22, 2024

I don't remember any more about the details of the code changes.
The "goal" is still to be able later to implement this:

(require-r-package '[datasets])
(r->clj r.datasets/iris {:as-tensor true})

so we can offer options to change the build-in conversions.

@behrica
Copy link
Member Author

behrica commented Sep 23, 2024

@genmeblog
I have a first versin of a working conversion to tensor.
See here for test:

( r->clj {:as-tensor true}))

Maybe have a quick look at the flow of the "options"

@behrica
Copy link
Member Author

behrica commented Sep 23, 2024

I have not checked if the "order" of elements is correct.

@behrica
Copy link
Member Author

behrica commented Sep 23, 2024

I have the impression that this ns
tech.v3.tensor.dimensions

contains the right functions to "convert" in one go from the "matrix data as seq" as we get it from
(prot/->clj exp)
into the tensor....

@behrica
Copy link
Member Author

behrica commented Sep 23, 2024

@genmeblog
I noticed that for R arrays we alreday get a flat vector from REngine.
Is it somehwere documented whats the order of this is in case of multidimensional arrays ?

@behrica
Copy link
Member Author

behrica commented Sep 23, 2024

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

Successfully merging this pull request may close these issues.

2 participants