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.

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

Add matrix class in clad #609

Merged
merged 3 commits into from
Aug 4, 2023
Merged

Conversation

vaithak
Copy link
Collaborator

@vaithak vaithak commented Jul 31, 2023

This class will be used for adding support for arrays in vector mode.

@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #609 (4848d33) into master (ad2b804) will not change coverage.
The diff coverage is n/a.

❗ Current head 4848d33 differs from pull request most recent head c8e7b4e. Consider uploading reports for the commit c8e7b4e to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #609   +/-   ##
=======================================
  Coverage   93.73%   93.73%           
=======================================
  Files          41       41           
  Lines        5939     5939           
=======================================
  Hits         5567     5567           
  Misses        372      372           

Copy link
Collaborator

@parth-07 parth-07 left a comment

Choose a reason for hiding this comment

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

Overall looks good.

include/clad/Differentiator/Matrix.h Outdated Show resolved Hide resolved
Copy link
Owner

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Modulo minor comments this looks good to me! Let's address them and merge it.

benchmark/Matrix.cpp Outdated Show resolved Hide resolved
include/clad/Differentiator/Array.h Show resolved Hide resolved
include/clad/Differentiator/Matrix.h Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

include/clad/Differentiator/Matrix.h Outdated Show resolved Hide resolved
include/clad/Differentiator/Matrix.h Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

include/clad/Differentiator/ArrayRef.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ArrayRef.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ArrayRef.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ArrayRef.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ArrayRef.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ArrayRef.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ArrayRef.h Outdated Show resolved Hide resolved
include/clad/Differentiator/ArrayRef.h Outdated Show resolved Hide resolved
@vaithak
Copy link
Collaborator Author

vaithak commented Aug 2, 2023

I have removed the special handling of creating clad::arrays without copying and instead used array_ref in such cases because it was leading to more confusion.

@vgvassilev
Copy link
Owner

I have removed the special handling of creating clad::arrays without copying and instead used array_ref in such cases because it was leading to more confusion.

Agreed. I was about to ask that question in the review but was carried away...

Another generic thing to consider is that eventually we will need to add C support.

@vaithak
Copy link
Collaborator Author

vaithak commented Aug 2, 2023

Another generic thing to consider is that eventually we will need to add C support.

Can you elaborate on what you mean by adding C support?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev
Copy link
Owner

Another generic thing to consider is that eventually we will need to add C support.

Can you elaborate on what you mean by adding C support?

Very soon clad should start supporting the C language. We should probably be thinking about that when implementing new features in clad.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

clang-tidy review says "All clean, LGTM! 👍"

@vaithak
Copy link
Collaborator Author

vaithak commented Aug 4, 2023

Is this good to go, or are any changes required?

Copy link
Owner

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

@vgvassilev
Copy link
Owner

The bot failures are not due to this PR. However, we should really look into the bot failures...

@vgvassilev vgvassilev merged commit 1d8f664 into vgvassilev:master Aug 4, 2023
68 of 75 checks passed
@vaithak vaithak deleted the clad-matrix branch August 9, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants