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

To array and to sparse functions are ready for linearoperator #368

Open
wants to merge 13 commits into
base: devel
Choose a base branch
from

Conversation

mateomarin97
Copy link

The default toarray and tosparse functions have been added to linearoperator.
Also, parallel tests for these functions have been written.

@yguclu
Copy link
Member

yguclu commented Oct 9, 2024

@mateomarin97 We see that you have made changes to the code after Elena's questions, but it is not clear what exactly they are for. Could you please answer explicitly?

@mateomarin97
Copy link
Author

@mateomarin97 We see that you have made changes to the code after Elena's questions, but it is not clear what exactly they are for. Could you please answer explicitly?

Hello. The changes were just aesthetic. I made the code more readable by implementing the suggestions Elena mentioned. The only thing stopping this function from being ready is that for the sparse matrix case, I could not write a Pyccel kernel to replace the for loop that appends the new matrices entries since Pyccel did not support Python lists.

As the code stands it works but it is too slow due to this problem.

@mateomarin97
Copy link
Author

@mateomarin97 We see that you have made changes to the code after Elena's questions, but it is not clear what exactly they are for. Could you please answer explicitly?

Hello. The changes were just aesthetic. I made the code more readable by implementing the suggestions Elena mentioned. The only thing stopping this function from being ready is that for the sparse matrix case, I could not write a Pyccel kernel to replace the for loop that appends the new matrices entries since Pyccel did not support Python lists.

As the code stands it works but it is too slow due to this problem.

Now that I think about it I am fairly certain I have a newer version of this branch that I did not push. I will push it later.

Copy link
Contributor

@e-moral-sanchez e-moral-sanchez left a comment

Choose a reason for hiding this comment

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

For clarity and to avoid repetition, I would group the lines within a bunch of if BoS =='b' elif ...

'The codomain of the LinearOperator must be a BlockVectorSpace or a StencilVectorSpace.')

if BoS == "b":
comm = self.domain.spaces[0].cart.comm
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to write this line right after 255 (within the if of 254). Same comment for the rest of if's.

Copy link
Author

Choose a reason for hiding this comment

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

I have done as requested

itterables2 = []
for ii in range(ndim2):
itterables2.append([starts2[ii], ends2[ii]+1])
#itterables2.append(range(starts2[ii], ends2[ii]+1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please clean the comments in the code

Copy link
Author

Choose a reason for hiding this comment

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

Ok. I will clean up the code and take this into consideration

Copy link
Author

Choose a reason for hiding this comment

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

It has been done.

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