Skip to content

Use d_prototype when solving PDSProblems #99

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Use d_prototype when solving PDSProblems #99

wants to merge 4 commits into from

Conversation

SKopecz
Copy link
Collaborator

@SKopecz SKopecz commented Jul 15, 2024

All (SSP)MPRK algorithms use d_prototype to initialize destruction vectors when solving a PDSProblem.

@SKopecz
Copy link
Collaborator Author

SKopecz commented Jul 15, 2024 •
edited
Loading

TODO:

  • Add tests
  • Update doc strings

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2024 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.24%. Comparing base (4fdc654) to head (2bf3d06).

Files Patch % Lines
src/mprk.jl 90.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   98.32%   98.24%   -0.08%     
==========================================
  Files           6        6              
  Lines        1250     1257       +7     
==========================================
+ Hits         1229     1235       +6     
- Misses         21       22       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SKopecz SKopecz linked an issue Jul 17, 2024 that may be closed by this pull request
Use d_prototype in PDSProblems #97
Closed
@SKopecz SKopecz requested a review from ranocha July 17, 2024 12:26
@ranocha
Copy link
Member

ranocha commented Jul 17, 2024

Risking to be annoying for you, I have thought a bit more about the d_prototype issue. In the end, I think you are probably right and we do not need the p_prototype for now - we should just use similar(u0) (instead of zeros(eltype(u0), length(u0))). This also enables GPU arrays and stuff like that. Moreover, I do not really see that using a sparse version of the destruction terms will be crucial for performance since the linear system solves will dominate everything. Thus, it will likely be better to keep a simpler structure to reduce the amount of code we have to maintain.

If you agree and are not too annoyed by my premature thoughts earlier, we can keep the improvements to the docs for d_prototype from this PR - maybe moving that to another PR and mark this one here as draft for future reference? Then, we should remove the d_prototype stuff in another PR which brings a breaking change (since we change the documented API).

ranocha
ranocha reviewed Jul 17, 2024
View reviewed changes
src/proddest.jl
@@ -20,10 +20,10 @@ The functions `P` and `D` can be used either in the out-of-place form with signa

### Keyword arguments: ###

- `p_prototype`: If `P` is given in in-place form, `p_prototype` is used to store evaluations of `P`.
- `p_prototype`: If `P` is given in in-place form, `p_prototype` or copies thereof are used to store evaluations of `P`.
Copy link
Member

Choose a reason for hiding this comment

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

We should definitely merge these documentation clarifications!

SKopecz
SKopecz commented Jul 17, 2024
View reviewed changes
test/runtests.jl
@@ -1263,6 +1263,84 @@ end
end
end

# Here we check that the types of p_prototype and d_prototype actually
Copy link
Collaborator Author

@SKopecz SKopecz Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess we should also keep this test (without the d_prototype parts) ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes 👍

@SKopecz
Copy link
Collaborator Author

SKopecz commented Jul 17, 2024

If you agree and are not too annoyed by my premature thoughts earlier, we can keep the improvements to the docs for d_prototype from this PR - maybe moving that to another PR and mark this one here as draft for future reference? Then, we should remove the d_prototype stuff in another PR which brings a breaking change (since we change the documented API).

Okay, let's do this.

@ranocha ranocha marked this pull request as draft July 17, 2024 14:44
@SKopecz SKopecz removed a link to an issue Jul 17, 2024
Use d_prototype in PDSProblems #97
Closed
@SKopecz SKopecz mentioned this pull request Jul 17, 2024
Merged
@SKopecz
Copy link
Collaborator Author

SKopecz commented Jul 17, 2024

This draft will not be pursued further. See #104 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ranocha ranocha ranocha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SKopecz @codecov-commenter @ranocha