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

Added conversion for two-dimensional arrays #228

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

sebasguts
Copy link
Contributor

No description provided.

@sebasguts sebasguts force-pushed the sg/two_dim_conversion branch from 06a741f to 0eafe08 Compare April 18, 2019 15:17
@sebasguts sebasguts marked this pull request as ready for review April 18, 2019 15:17
@codecov
Copy link

codecov bot commented Apr 18, 2019

Codecov Report

Merging #228 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #228   +/-   ##
=======================================
  Coverage   75.21%   75.21%           
=======================================
  Files           5        5           
  Lines         456      456           
=======================================
  Hits          343      343           
  Misses        113      113

Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

The two typos must be fixed.
Besides that, eventually the documentation will have to describe the effects of recursive conversions;
as far as I see, equal subobjects on the source side are turned into identical objects on the target side.

new_array = Array{T,2}( undef, len_list_outer, len_list_inner )
recursion_dict[obj] = new_array
for i in 1:len_list_outer
for j in 1:len_list_outer
Copy link
Member

Choose a reason for hiding this comment

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

1:len_list_inner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for spotting this, fixed now

new_array = Array{Union{Nothing,T},2}( undef, len_list_outer, len_list_inner )
recursion_dict[obj] = new_array
for i in 1:len_list_outer
for j in 1:len_list_outer
Copy link
Member

Choose a reason for hiding this comment

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

1:len_list_inner

@fingolfin
Copy link
Member

as far as I see, equal subobjects on the source side are turned into identical objects on the target side.

What makes you say that? This would be a bug in general. Do you have some specific examples for this?

@sebasguts sebasguts force-pushed the sg/two_dim_conversion branch from 0eafe08 to 18e0348 Compare April 20, 2019 11:08
@sebasguts
Copy link
Contributor Author

I think this is a misunderstanding. Identical objects on the source side are turned into identical objects (on purpose). This is done by using IdDict(), which compares by ===, not ==, for caching already converted objects.

Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@ThomasBreuer
Copy link
Member

Thanks for the explanation.

@sebasguts sebasguts merged commit a2b3cfb into oscar-system:master Apr 23, 2019
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