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

Switch from proto to R6 for all classes #44

Closed
Martin-Jung opened this issue Apr 4, 2023 · 8 comments
Closed

Switch from proto to R6 for all classes #44

Martin-Jung opened this issue Apr 4, 2023 · 8 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request priority Any issue that should be implemented or fixed soon'ish

Comments

@Martin-Jung
Copy link
Collaborator

There is some indication that this will have benefits in terms of memory efficiency. Thus we should at some point switch to objects defined by the R6 package instead.

@Martin-Jung Martin-Jung added enhancement New feature or request low priority labels Apr 4, 2023
@Martin-Jung Martin-Jung added the wontfix This will not be worked on label Jun 14, 2023
@Martin-Jung
Copy link
Collaborator Author

Martin-Jung commented Jun 14, 2023

To do at a later point. Apparently R7 is the newest hot stuff but has not yet matured to a post-beta version.
Transition as soon as R7 is on CRAN and remove the proto dependency.

@Martin-Jung Martin-Jung added priority Any issue that should be implemented or fixed soon'ish documentation Improvements or additions to documentation and removed wontfix This will not be worked on low priority labels Jan 22, 2024
@Martin-Jung Martin-Jung self-assigned this Jan 24, 2024
Martin-Jung added a commit that referenced this issue Jan 24, 2024
Martin-Jung added a commit that referenced this issue Jan 24, 2024
Martin-Jung added a commit that referenced this issue Jan 24, 2024
* Class type conversions from proto to R6 and documentation #44

* Turning zoom meetings into documentation #44

* All proto classes renamed and removed #44

* All bdproto calls removed throughout #44

* `bdproto` now also from engines removed #44

* All tests solve without issues #44

* Minor doc fixes before merge #44
@Martin-Jung
Copy link
Collaborator Author

Closed and implemented now in dev
FYI @mhesselbarth changed all class structure to R6 from proto. Still some notes to resolve (missing self object) but otherwise this will hopefully be cleaner.
We also have now basic definitions for class-functions, e.g. see https://iiasa.github.io/ibis.iSDM/reference/DistributionModel-class.html

@mhesselbarth
Copy link
Collaborator

Perfect! I still have some unmerged changes on a local branch fixing a few of the notes. Will look into this next week how to merge.

@mhesselbarth
Copy link
Collaborator

mhesselbarth commented Feb 23, 2024

So R6 objects get changed outside the scope of a function? So for example

ibis.iSDM::add_biodiversity_poipa(x = base_model, poipa = ebird_pa, weight = 1,
           field_occurrence = "individualCount")

would change the object base_model even without assigning it?

This seems to be an issue for some functions (e.g., write_model).

Also, the behavior is not consistent across functions I think. For example train does acutally not change it original object

@mhesselbarth mhesselbarth reopened this Feb 23, 2024
@mhesselbarth
Copy link
Collaborator

Also, it seems that the class information about the trained engine is missing now for DistributionModel. Needs to be added again

@Martin-Jung
Copy link
Collaborator Author

So R6 objects get changed outside the scope of a function? So for example

ibis.iSDM::add_biodiversity_poipa(x = base_model, poipa = ebird_pa, weight = 1,
           field_occurrence = "individualCount")

would change the object base_model even without assigning it?

This seems to be an issue for some functions (e.g., write_model).

Also, the behavior is not consistent across functions I think. For example train does acutally not change it original object

I found a similar behaviour for rm_predictors. This is related to object inheritance and unfortunately R6 is a bit more itchy with this, essentially requiering a deep clone of each object when modifying the contents.

@Martin-Jung
Copy link
Collaborator Author

Also, it seems that the class information about the trained engine is missing now for DistributionModel. Needs to be added again

Yes, but I think I changes this throughout so that the class information is saved as public (or private) attribute instead. If there are still come functions in the package that do a class check, this needs to be changed.

@Martin-Jung
Copy link
Collaborator Author

Ok, fixed both issues including the class import check in load_model and added unit tests in 9eff17b
"Class" (or rather algorithm) names can be queried via mod$get_name().
train() or project() should not change the original object I think.

Reopen the issue if you think there is something else that did not work following the R6 transition.

Martin-Jung added a commit that referenced this issue Feb 28, 2024
* 🚋 ride for 🐛 fixing in threshold and ensemble

* Update CITATION.cff

* Small fix with weighted mean and multi-band rasters

* gdalUtilities switch, removing PCA for now owing to unit test issues downstream.

* Fixing previous 🐛 introduced through controls revamping

* Make sure partial is consistent across methods and works for several x.var

* Kleines Upsi mit aes und ggplot2

* Bugfix in partial gdb when only_linear = F

* x axis label for partial

* All x.var = NULL for partial()

* Make ribbons easier to see

* Inconcistency in partial breg for newdata

* Some minor fixes partial engines

* Fix and clean gdb partial

* Fix stan partial

* Cln docs (#85)

* Remove usage (constructed automatically)

* Update globals

* Fix truncated examples

* Remove exportMethod (export should be enough)

* Fixing and cleaning docs for RCMD

* Some re-ordering and cleaning of DESCRIPTION and ibis.iSDM-package.R

* Remove @method tag (roxygen should be able to figure it out itself)

* Added @method were actaully needed

* Remove not need @import sf

* Cleaner ImportFrom

* Small cosmetic changes to docs

* Revert "Small cosmetic changes to docs"

This reverts commit 6265e89.

* Small cosmetic doc changes

* Make internals easier to find

* Keywords dont need a comma

* Fixed introduced bug in examples

* sf and stars dont export their classes, so we cant use @importClassesFrom

* Update NEWS

* I dont think we need to export the setOldClass call?

* Only include scripts that define classes and methods

* Make sure pgkdown.yml still runs

* Update CITATION.cff

* Remove skip_on_travis (deprecated)

* Added parameter to allow threshold compositing #84

* Changing class and objects to R6 throughout  (#88)

* Class type conversions from proto to R6 and documentation #44

* Turning zoom meetings into documentation #44

* All proto classes renamed and removed #44

* All bdproto calls removed throughout #44

* `bdproto` now also from engines removed #44

* All tests solve without issues #44

* Minor doc fixes before merge #44

* Update CITATION.cff

* Small documentation fix and vignette fix for runner

* Documentation update and `rm_biodiversity` addition.

* fix #91

* close #92

* Use update.formula to be consistent

* Clone model before writting to disk.

Bug in load_model due to missing engine class (outcommented for now)

* Adding wrap_model function

* Fix for object inheritance and class checks #44

* Added default parameters for all ibis specific options #90

* No warning thin_obs removing ojbects

* Small 🐛 fix to `write_output`

* 🐛 Adding 1e-15 to y_pred in logloss

* Changing behaviour of weights in `engine_inlabru()` #93

---------

Co-authored-by: Martin-Jung <Martin-Jung@users.noreply.github.com>
Co-authored-by: mhesselbarth <mhk.hesselbarth@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request priority Any issue that should be implemented or fixed soon'ish
Projects
None yet
Development

No branches or pull requests

2 participants