Github user actuaryzhang commented on the issue:
https://github.com/apache/spark/pull/16699
@imatiach-msft Many thanks again for the review. I have incorporated some
of your suggestions:
1. Create initialization of instance directly if it is Gaussian(identity)
to avoid expensive conversion from OffsetInstance.
2. Fix spacing issues in test
3. Change `off` to `offset` to be more informative.
Feedback on the other suggestions:
1. `move import to beginning, use ~==`: I did not change this for
consistency with other tests in GLM. It's probably better to update all tests
in a separate PR if this is preferred.
2. Inheriting Instance: I'm not sure about this and leaning toward not
touching `Instance` since it is used a lot elsewhere. Let's see what others
suggest.
Question: if `OffsetInstance` is a subclass of `Instance`, will a method
with a signature `RDD[Instance]` also work with `RDD[OffsetInstance]`?
3. Would you please elaborate on the following comment:
> if family = Gaussian and link = Identity we shouldn't be passing any
offset.
This is the linear regression case and it still allows `offset`. The offset
will be subtracted from the response as can be seen from the initialization in
WLS `Instance(label - offset, weight, features)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
