-
Notifications
You must be signed in to change notification settings - Fork 16
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
1167 Update some dependencies #1168
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1168 +/- ##
==========================================
+ Coverage 96.95% 96.97% +0.01%
==========================================
Files 148 148
Lines 13715 13710 -5
==========================================
- Hits 13298 13295 -3
+ Misses 417 415 -2 ☔ View full report in Codecov by Sentry. |
@reneSchm Please update readmes :) |
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.
Thank you for the updates. I gave some more or less informed comments. Maybe check for them, update the readmes and then it should be OK.
* Resize all dimensions. | ||
* @param new_dims new dimensions. | ||
* @brief Resize all dimensions. | ||
' Note that when increasing the overall size, new values may be uninitialized. |
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.
' Note that when increasing the overall size, new values may be uninitialized. | |
* Note that when increasing the overall size, new values may be uninitialized. |
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.
what happens when reducing multi-dim arrays? Is the form kept or do information get mixed?
E.g.
3x3 of
1 2 3
4 5 6
7 8 9
changing to 2x2 becomes
1 2
4 5
or rather
1 2
3 4
?
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.
The internal array is stored linearly and reduced (or extended) to the product of all dimensions. So your 3x3 matrix is really just an array (1 2 3 4 5 6 7 8 9), with matrix accessor. So resizing to 1x9 or 9x1 would leave the array untouched. If we cut that matrix to
- 3x2, we get
1 2
3 4
5 6 - 3x1, we get
1
2
3 - 2x3, we get
1 2 3
4 5 6 - 2x2, we get
1 2
3 4
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.
I was expecting that. Is that really expected behavior for a (new) user? Shouldn't we avoid sizing down with keeping the values as this is normally not what you what want when reducing dimensions?
Thanks for the review @mknaranja. Please check the "additional information for reviewers", in case you haven't. I had made Eigen a system library, so we do not need to manually add compiler diagnostics. I hadn't removed them before your review (see here). I also replaced mio::Vector by Eigen::VectorX (new in 3.4.0) in that commit, so it is more obvious that Vector is a Eigen type and not something we wrote ourselves. |
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.
Looks good. There is just one comment / discussion we should agree on.
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
If need be, add additional information and what the reviewer should look out for in particular:
Merge Request - Guideline Checklist
Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.
Checks by code author
Checks by code reviewer(s)
Closes #1167