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.

Sign up for GitHub

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

Jump to bottom

operands_and_partials refactor #547

Merged
merged 44 commits into from
May 24, 2017
Merged

operands_and_partials refactor #547

Changes from 1 commit
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
74464c8
Refactor scalar_type and value_type
seantalts Mar 27, 2017
d09a3a2
Don't need consts here.
seantalts Mar 28, 2017
0f63e72
Temp - multivariate working, needs fvar
seantalts Mar 30, 2017
c927e44
Temp2 - show Rob. Started getting everything working for fwd mode, ne…
seantalts Apr 11, 2017
a9cc0b3
Get fwd/mat test passing.
seantalts Apr 17, 2017
c341d7a
Get fvar/scal test working
seantalts Apr 17, 2017
cc99757
Fix mixed mode
seantalts Apr 18, 2017
9361833
more fixes for mixed mode
seantalts Apr 19, 2017
7116fb2
Adding a temporary test to hunt down seg fault
Apr 20, 2017
7e343b9
Fix segfault by initializing partials for fvar<var> etc
seantalts Apr 20, 2017
6d94a24
Merge branch 'develop' of github.com:stan-dev/math into ops_partials
seantalts Apr 26, 2017
2eeb9e6
Get all the unit tests passing with the new API
seantalts May 3, 2017
2366e49
Replace OperandsAndPartials uses; remove it and VectorView
seantalts May 3, 2017
52761b9
cpplint
seantalts May 3, 2017
61dae11
Merge branch 'develop' of github.com:stan-dev/math into cleanup/546-o…
seantalts May 3, 2017
739c3fd
Fix includes.
seantalts May 4, 2017
a7bfeb1
Fix mixed mode tests. Remove unused tests.
seantalts May 5, 2017
d249fa0
Add test for return_type
seantalts May 5, 2017
8c196ed
Merge branch 'develop' of github.com:stan-dev/math into cleanup/546-o…
seantalts May 6, 2017
56ebec5
Add doc
seantalts May 6, 2017
f4bdc5e
Merge branch 'develop' of https://github.com/stan-dev/math into clean…
seantalts May 8, 2017
2c4d302
Fix broadcast_array test to use new fvar ctor
seantalts May 8, 2017
848b3c3
Address Bob's comments.
seantalts May 9, 2017
27907f4
Merge branch 'cleanup/546-operands-partials' of github.com:stan-dev/m…
seantalts May 9, 2017
c2b5b2f
Fix whitespace errors introduced by _
seantalts May 9, 2017
f18697e
Fix last missing _
seantalts May 9, 2017
3ef7cdc
Address Dan's pull request comments
seantalts May 10, 2017
fcbcbb8
Add mixed mode multivariate test
seantalts May 10, 2017
2dece4d
Fix mixed multivariate operands_and_partials
seantalts May 10, 2017
1afac1b
Fix cpplint error.
seantalts May 10, 2017
65f894d
Don't need container view anymore -
seantalts May 10, 2017
b0b7329
Fix multivariate mixed tests
seantalts May 11, 2017
eaa147b
New test for creating zero matrices and vectors
seantalts May 11, 2017
e1d06bd
Use friendship to hide edge internals from stan::math namespace.
seantalts May 16, 2017
3b0e04c
Switch from a 'detail' namespace to 'internal'
seantalts May 16, 2017
a0e7d1f
Deal with empty operands_ appropriately.
seantalts May 16, 2017
03aecb0
Fix cpplint
seantalts May 16, 2017
9044360
Unlikely.
seantalts May 16, 2017
e71f0d5
Fix doc up a bit, more doxygen
seantalts May 18, 2017
4131e23
[ci-skip] Start addressing Bob's comments; sync
seantalts May 19, 2017
f904dc0
Refactor to avoid using inheritance for clarity's sake
seantalts May 19, 2017
be58571
Get rid of ViewElt
seantalts May 20, 2017
f9863b8
Need ViewElt still for arithmetic primitive edges
seantalts May 20, 2017
ae81a2e
cpplint
seantalts May 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Deal with empty operands_ appropriately.
  • Loading branch information
@seantalts
seantalts committed May 16, 2017
commit a0e7d1f65be8799848b9637bacdc931cb9c104c2
1 change: 1 addition & 0 deletions stan/math/prim/mat/meta/operands_and_partials.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ namespace stan {
friend class stan::math::operands_and_partials;
const Ops& operands_;
int size() {
if (this->operands_.size() == 0) return 0;
return this->operands_.size() * this->operands_[0].size();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: This relies on operands_ to have a size greater or equal to 1. I'm not sure how this is going to behave when operands_ is size 0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does that happen?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happens when ops is 0 length. That could be an Eigen::Matrix<var, -1, -1> with either rows == 0 or columns == 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of fixing the code, please doc that this will break if operands_ is of size 0. In our current use cases, this isn't possible, but I can imagine use cases in the future where this will be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use the following to avoid the problem?

return this->operands_.size() == 0 ? 0 : this->operands_.size() * this->operands_[0].size();

I agree that this will never happen with our current usage. I don't think the switch overhead will be that high here. We could even go with

if (expected(this->operands_.size() > 0)
  return this->operands_.size() * this->operands_[0].size();
else
  return 0;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did basically this (though I think it was called unlikely).

}
};
Expand Down