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

Bug in outer constructor of Table{...} for input arrays of abstract t… #285

Merged
merged 2 commits into from
Jun 22, 2020

Conversation

amartinhuertas
Copy link
Member

…ype.

@codecov-commenter
Copy link

Codecov Report

Merging #285 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #285   +/-   ##
=======================================
  Coverage   88.31%   88.31%           
=======================================
  Files         153      153           
  Lines       10044    10044           
=======================================
  Hits         8870     8870           
  Misses       1174     1174           
Impacted Files Coverage Δ
src/Arrays/Tables.jl 87.05% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f333786...c634148. Read the comment docs.

Copy link
Member

@fverdugo fverdugo left a comment

Choose a reason for hiding this comment

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

@amartinhuertas

Thanks for the PR!

Just a comment: be aware that this fix might allocate new vectors. Perhaps in some contexts instead of using a Table, one could implement and use a more tailored specialization of a vector of vectors in order to avoid this.

@fverdugo fverdugo merged commit 533b99a into master Jun 22, 2020
@fverdugo fverdugo deleted the bugfix_in_table_outer_constructor branch June 22, 2020 06:10
@amartinhuertas
Copy link
Member Author

Just a comment: be aware that this fix might allocate new vectors.

Yes, sure!

Perhaps in some contexts instead of using a Table, one could implement and use a more tailored specialization of a vector of vectors in order to avoid this.

Yes. Indeed, due to the fact that members of Table are of type Vector, then Table cannot serve our original purpose in GridapDistributed.jl (i.e., a vector of vectors build as a view of a one dimensional vector). I had to apply an alternative solution not based in the use of Table. In any case, I applied boy scout's rule to fix this issue of Table that I found while reading the code bound to Table.

@fverdugo
Copy link
Member

👍
If you have created some struct that is general enough, we can also include it in Gridap so that we can use it in other projects

@amartinhuertas
Copy link
Member Author

If you have created some struct that is general enough, we can also include it in Gridap so that we can use it in other projects

I did not have to create an special structure. Three lines of code are sufficient:

gridap/GridapDistributed.jl@e98d5f4#diff-fb2be2846b38d56cdaf599d624ece152R76

If u feel that this is going to be a recurrent/useful feature, then we can perhaps develop a type extension of AbstractVector

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