-
Notifications
You must be signed in to change notification settings - Fork 36
add zero(::ColVecs) / zero(::RowVecs) definitions #444
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #444 +/- ##
===========================================
- Coverage 93.13% 43.23% -49.90%
===========================================
Files 52 52
Lines 1252 1242 -10
===========================================
- Hits 1166 537 -629
- Misses 86 705 +619
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some tests? Otherwise looks good to me.
Look nice but maybe add some unit tests? |
Is this PR so important that you can't even wait for me to recheck whether I'm happy with the changes and officially approve? 😄 🙈 👀 I really don't think this is how it should work. |
I really don't think you should be blocking PRs on minor details such as whether a function in a test is in one place or another. That seems really petty. I was waiting on this so I could fix it over in AbstractGPs. It seems like any time you look at something you find yet another detail you can criticise that isn't yet as perfect as you can dream it up. And yes, on the whole your code is amazing, and you can make really good comments. But requesting changes on minor details like this really puts me off contributing at all. You previously said not to expect reviews from you. Please don't review my code then, and leave it to @willtebbutt / @theogf / ... |
This is just ridiculous - all suggestions here were reasonable, responsive, and ended up in the code, in some way or another. I would have approved the PR anyway today, the main point is just that I don't see how merging PRs before reviewers were able to recheck is good behaviour. Well, I'm out here and giving up on you. I've tried to discuss and fix these collaborations long enough and have better things to do if you don't plan to work on it. |
Yes, and the same applies in reverse - I've tried to discuss & improve these collaborations too! I'm just really at a loss how to get across how upset I get and how much I struggle with the way you give feedback on PRs. I'd be happy to discuss what we could both do differently, if it's not just "how do you plan to work on it" but also how do you plan to work on it. In the meantime, maybe better if you don't look at my PRs and I don't look at your PRs, and either can be reviewed by other people involved in the packages (@theogf / @willtebbutt / etc. as appropriate). Can we do that? |
I only try to provide valuable suggestions, in the same way I get and I would like to get suggestions on my PRs. But since clearly this is not desired, leads to endless unproductive discussions, and I don't see how this will change in the near future, I won't review any of your PRs anymore and hope others, possibly with more time and less frustration, provide you with the same suggestions and comments. In general, I will refrain from any further interaction with you, it's too time consuming and unproductive. As a final possibly not well received comment, maybe you should start considering that the problem is on your side (as well) since reviews, discussions, etc. work and worked fine with basically all other people on Github for me. I unsubscribed from this PR. |
Moved here from the AD tests in JuliaGaussianProcesses/AbstractGPs.jl#313