[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-195258085
  
Returning a value by  modifying an arg in place is exceptional, but I don't 
think it's something that can never happen. This is a case where it should, and 
it's done and documented in this special-purpose implementation intentionally 
for performance. I don't know about you, but around BLAS/lapack and related 
methods is exactly where I expect the function args to be modified, since 
that's the only way lapack works.

Ideally, the modifying in-place semantics is reusable for a bunch of 
purposes. Since it's just the result of the lapack call, I suspect it could be. 
However that doesn't mean it's suitable for your use case. But if your solution 
is to clone for all calls, then a solution for this particular other usage is 
to pass a clone of your argument, yeah.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[ehsanmok]

Github user ehsanmok closed the pull request at:

https://github.com/apache/spark/pull/11617


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[ehsanmok]

Github user ehsanmok commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-195078271
  
@srowen Ayway, I see there might not be a major problem.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[ehsanmok]

Github user ehsanmok commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-195074143
  
@srowen I'm referring to private[spark] BLAS object in BLAS.scala. Well, 
the solution is not the point here, my point is that the implementation 
standards must be consistent across all mllib (and spark). We can't have side 
effects in some of them and remember to clone those every time but don't do 
that for other objects. I hope it's clear now!


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-195071510
  
Which bit are you referring to when you say BLAS? the libs or the class? 
Yes I understand you're also using it internally, which means the semantics 
only really matter inside the project. The docs already say these arrays are 
modified in place. You have a working solution -- at worst, you clone.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[ehsanmok]

Github user ehsanmok commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-195066706
  
Well, if it's about performance shouldn't the same apply to BLAS then? and 
the jira I mentioned is not something outside of Spark, it's going to be on top 
of mllib.linalg.optimization. Cloning the array is the solution but not a 
consistent one with the whole Spark as far as i can tell and developer should 
always do that for CholeskyDecomposition but not for BLAS! That's the double 
standard. And accordingly docs and those javaALS tests must be modified.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194999543
  
I presume the motivation is simply speed and not copying the array. This 
class is not meant to be used outside Spark, so I don't think its API needs to 
meet needs except Spark's. In the other usage for the other JIRA, you can just 
copy the array before passing it in right? It is documented and as far as I can 
tell on purpose.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[ehsanmok]

Github user ehsanmok commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194995065
  
@srowen If this is not a bug now, it's at least a major issue in my 
opinion. I don't understand the double standard, there're exactly side-effects 
that captured correctly in BLAS but not here! I've spent a day tracking down 
the bug during my implementation of 
[#6417](https://issues.apache.org/jira/browse/SPARK-6417) that uses 
CholeskyDecomposition.solve two times in each iteration on a fix matrix but the 
matrix was changing unnoticeably. It's not about what doc says, it's about what 
is right from functional style!


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194713989
  
Certainly, the lapack methods modify their arguments in order to return a 
value. That's by design of course. And the javadoc for this method indicates 
that the `solve()` method also intends to modify its args in place. Your tests 
show this is required. This isn't a bug then.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[ehsanmok]

Github user ehsanmok commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194595529
  
@jkbradley @mengxr According to lapack docs there're clear side effects but 
since CholeskyDecomposition was used in ALS those tests are failed, leaving me 
in wonder why? are side effects being used there which shouldn't since it's a 
separate internal implementation?


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194590426
  
Merged build finished. Test FAILed.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194590427
  
Test FAILed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/52783/
Test FAILed.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194590370
  
**[Test build #52783 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52783/consoleFull)**
 for PR 11617 at commit 
[`894e8ec`](https://github.com/apache/spark/commit/894e8ecdd0a7101bec575b2a8090b996d5d73d12).
 * This patch **fails Spark unit tests**.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194581844
  
**[Test build #52783 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52783/consoleFull)**
 for PR 11617 at commit 
[`894e8ec`](https://github.com/apache/spark/commit/894e8ecdd0a7101bec575b2a8090b996d5d73d12).


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194571923
  
Build finished. Test FAILed.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194571926
  
Test FAILed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/52776/
Test FAILed.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194571865
  
**[Test build #52776 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52776/consoleFull)**
 for PR 11617 at commit 
[`86a47bd`](https://github.com/apache/spark/commit/86a47bdee06ec5ae0b6e664211498c18ec2b3b53).
 * This patch **fails MiMa tests**.
 * This patch **does not merge cleanly**.
 * This patch adds no public classes.


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/11617#issuecomment-194566551
  
**[Test build #52776 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52776/consoleFull)**
 for PR 11617 at commit 
[`86a47bd`](https://github.com/apache/spark/commit/86a47bdee06ec5ae0b6e664211498c18ec2b3b53).


---
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

[GitHub] spark pull request: [SPARK-13788][MLLIB] Fix side effects in Chole...

[ehsanmok]

GitHub user ehsanmok opened a pull request:

https://github.com/apache/spark/pull/11617

[SPARK-13788][MLLIB] Fix side effects in CholeskyDecomposition

## What changes were proposed in this pull request?

Capturing lapack side effects by cloning arrays.

## How was this patch tested?

Manual test as well as checking with lapack documentations: 

http://www.netlib.org/lapack/explore-html/d3/d62/dppsv_8f.html
http://www.netlib.org/lapack/explore-html/d2/de0/dpptri_8f.html

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ehsanmok/spark JIRA-13788

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/11617.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #11617


commit 86a47bdee06ec5ae0b6e664211498c18ec2b3b53
Author: Ehsan M.Kermani 
Date:   2016-03-09T23:22:38Z

capturing lapack side effects with cloning arrays




---
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