[R-package] avoid unnecessary computation and add tests for Dataset set_reference() method

[jameslamb]

Noticed while working on #4586.

Dataset$set_reference() allows changing the reference Dataset that a given Dataset is based on, which affects (for example) how features are binned.

Using Dataset$set_reference() to set reference to the existing object doesn't have any effect on the Dataset object. This PR proposes moving the existing check for that all the way up to the beginning of that method, to avoid any other unnecessary computation and slightly speed up this method for that case.

[StrikerRUS]

Why do we leave # Check for empty data and # Check for non-existing reference untouched placed after setting # Set known references. I think it is misleading that some attributes are set (categorical_feature, colnames and predictor) but the whole method fails with fatal error. Doesn't it leave object in a corrupted, ok maybe inconsistent, state?

Moreover, I might be wrong but

self$set_categorical_feature(categorical_feature = reference$.__enclos_env__$private$categorical_feature)

will simply crash without

        # Reference is unknown
        if (!lgb.is.Dataset(reference)) {
          stop("set_reference: Can only use lgb.Dataset as a reference")
        }

placed before in case of wrong reference type.

[jameslamb]

Ah yes, you're right! Didn't notice that those should also be moved up sooner. I'll make that change here.

[jameslamb]

This refactoring is also making me realize that that method doesn't protect against the case where you run set_reference(reference = NULL). Right now on master, that results in

Error in self$set_colnames(colnames = reference$get_colnames()) : 
  attempt to apply non-function

I'll fix that here too

[jameslamb]

Ok, just moved these validations up further. I realized while doing this that there are currently no direct tests on Dataset$set_reference(), so added some of to be more confident in this change.

You can confirm that there are no direct tests on that method by running the following on master.

git grep -E "set.*ref" R-package/tests

[jameslamb]

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/1201242577

Status: success ✔️.

[StrikerRUS]

With this PR, the order of code lines in set_reference() function looks good to me, thanks!
Just two minor comments regarding new tests.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.