Skip to content

Conversation

@dylan-copeland
Copy link
Collaborator

@dylan-copeland dylan-copeland commented Dec 21, 2023

The only real change in this PR is at line 887 of lib/mfem/SampleMesh.cpp in the master branch. Part of the verification on that line should be done only after the loop over all spaces. This is just a sanity check, and the changes in this PR should have no effect on any results. Other changes in this PR are just to clean up some unused variables, long lines, etc.

@dylan-copeland dylan-copeland self-assigned this Dec 21, 2023
@JacobLotz
Copy link
Collaborator

I have verified that this PR fixes issue #242. Thanks @dylan-copeland !

@dylan-copeland dylan-copeland added the RFR Ready for review label Dec 22, 2023
@dylan-copeland
Copy link
Collaborator Author

dylan-copeland commented Dec 22, 2023

Thanks @JacobLotz for the review. Sorry I did not see your issue. In the future, assigning me to an issue should help.

Copy link
Collaborator

@ckendrick ckendrick left a comment

Choose a reason for hiding this comment

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

This looks good. I just wanted to note that the unused variable std::unique_ptr<CAROM::BasisGenerator> basis_generator; also appears in PRs #249 #254 and #260. I'm not sure if it is better to remove this line in each of those PRs or this one.

@dylan-copeland
Copy link
Collaborator Author

This looks good. I just wanted to note that the unused variable std::unique_ptr<CAROM::BasisGenerator> basis_generator; also appears in PRs #249 #254 and #260. I'm not sure if it is better to remove this line in each of those PRs or this one.

Good point, that some other PRs also have this unused variable that keeps being copied to new examples. Since this PR is already approved, I think other PRs will either have the variable removed when merging with master, or they can remove their instances.

@dylan-copeland dylan-copeland merged commit 55a46a6 into master Jan 3, 2024
@dylan-copeland dylan-copeland deleted the smm-fix branch January 3, 2024 02:57
andersonw1 pushed a commit that referenced this pull request Jan 8, 2024
* Bug fix.

* Cleaning up a few things.
andersonw1 pushed a commit that referenced this pull request Apr 2, 2024
* Bug fix.

* Cleaning up a few things.
JacobLotz added a commit to JacobLotz/libROM-fork that referenced this pull request Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RFR Ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants