Skip to content

Conversation

@arrowguy234
Copy link
Contributor

This pull request corresponds to issue #70

pritkc
pritkc previously requested changes Feb 17, 2025
@mdumett mdumett self-requested a review February 21, 2025 19:28
@mdumett
Copy link
Collaborator

mdumett commented Feb 23, 2025

The sizes of the vectors and matrix consider 302 points and not 301. Please, run the MATLAB version to check the correct sizes of each array

@arrowguy234
Copy link
Contributor Author

arrowguy234 commented Feb 23, 2025

@pritkc All changes done

@mdumett
Copy link
Collaborator

mdumett commented Feb 27, 2025

Please, even though that is not in the comments of the MATLAB codes, indicate in the top comments of each of your codes which equation is being solved, its time (if required) and space domains and its initial (if required) and boundary conditions.

@arrowguy234
Copy link
Contributor Author

@mdumett Equation and conditions added professor.

@arrowguy234
Copy link
Contributor Author

@pritkc Hello , I had a request that if its possible can I get review of my code for the equations because I made the changes suggested.

Regards
Surinder

@pritkc
Copy link
Collaborator

pritkc commented Mar 18, 2025

@pritkc Hello , I had a request that if its possible can I get review of my code for the equations because I made the changes suggested.

Regards Surinder

You may have to reach out to @mdumett. In the meantime, you may retire your makefile and rebase your branch as we have moved to the new CMake system.

@arrowguy234
Copy link
Contributor Author

@pritkc @mdumett I have retired my old makefile and also tested my code on new Make system based in Ubuntu as well as WSL, @JananiPSrinivasan has verified the working.

@pritkc pritkc dismissed their stale review April 16, 2025 07:06

Rebase request fullfilled

@mdumett
Copy link
Collaborator

mdumett commented Apr 16, 2025

Use constexpr in the Burgers example.

Copy link
Collaborator

@jbrzensk jbrzensk left a comment

Choose a reason for hiding this comment

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

If you want to split this into 2 pull requests, the POisson2D seems good to go with the name change. Burgers1D needs more work.

@arrowguy234
Copy link
Contributor Author

@jbrzensk @mdumett ll requested changes have been completed.
In Burgers1D, terminal output now only prints example values instead of every line.
constexpr has been used where applicable.
The Poisson file has been renamed for clarity.
Let me know if there are any other suggestions or improvements needed.

@arrowguy234 arrowguy234 changed the title Minimal_Poisson2D and BurgersD example in C++ Burgers1D example in C++ Apr 20, 2025
@mdumett
Copy link
Collaborator

mdumett commented Apr 20, 2025

@arrowguy234: Burgers1D.m has a plot instead of examples values. Use gnuplot for plotting every 5 iterations. In addition, the matlab/octave code prints the value of the area under the curve using the trapezoidal rule which is missing in the C++ code. Implementing that function in C++ is not difficult. See https://en.wikipedia.org/wiki/Trapezoidal_rule . You can add that function in utils.cpp and utils.h

@arrowguy234
Copy link
Contributor Author

@mdumett Thanks a lot for your input, I have implemented the trapezoid rule in Utils file as well as included it in my Burgers1D code and along with that, the output is being printed in a graph plot like matlab, I'm happy to take on any additional suggestions or structural improvements you might have.

Copy link
Collaborator

@mdumett mdumett left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all comments

@mdumett
Copy link
Collaborator

mdumett commented Apr 24, 2025

Please, reuse D and that should be it.

@arrowguy234
Copy link
Contributor Author

@mdumett @aboada All suggested changes done, if anything looks out of place please suggest more changes.

@mdumett mdumett merged commit 5fa8ee5 into csrc-sdsu:master Apr 28, 2025
2 checks passed
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.

5 participants