Skip to content

ROM k-eigenvalue implementation#11

Open
quincy-huhn98 wants to merge 3 commits into
Open-Sn:mainfrom
quincy-huhn98:keff
Open

ROM k-eigenvalue implementation#11
quincy-huhn98 wants to merge 3 commits into
Open-Sn:mainfrom
quincy-huhn98:keff

Conversation

@quincy-huhn98

Copy link
Copy Markdown
Contributor

Contains my k-eigenvalue implementation as well as READMEs for each of the subfolders. I included an additional example in the workflow that runs a k-eigenvalue problem.

@@ -0,0 +1,14 @@
NUM_GROUPS 2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

OpenSn cross section files use usually .xs, not .txt

@@ -0,0 +1,27 @@
# 1dk/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is the trailing / part of the name or just copy-paste artifact?

}
if (rom_options.phase == Phase::SYSTEMS)
{
std::shared_ptr<CAROM::Matrix> AU_ = rom_problem_->AssembleAU();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If you are following coding standard for OpenSn, then local variables should not have trailing _.

* Each group basis is read from the corresponding libROM basis file and
* stored in \c Ugs_. The reduced dimension is inferred from the first
* basis and stored in \c rom_rank.
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Doxygen usually goes into the header file, not the .cc file.

But I see other functions have doxygen here, maybe that should be unified within the app in a separate PR.

@wdhawkins wdhawkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you haven't already, please run clang-tidy and flake8 on your changes.

assert(lbs_problem_->GetNumWGSSolvers() == 1);
auto raw_context = lbs_problem_->GetWGSSolver(0)->GetContext();
auto gs_context = std::dynamic_pointer_cast<WGSContext>(raw_context);
const auto scope = APPLY_AGS_FISSION_SOURCES | APPLY_WGS_FISSION_SOURCES | APPLY_AGS_SCATTER_SOURCES | APPLY_FIXED_SOURCES;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this just be APPLY_AGS_FISSION_SOURCES | APPLY_WGS_FISSION_SOURCES?

best_col = i;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should check here that you found a valid best_col and it's not still -1.

Comment thread ROM/python/plotting.py
errors = []
for g in range(G):
rom_vals[g] /= np.linalg.norm(rom_vals[g])
rom_vals = np.abs(rom_vals)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be better to use a local array here instead of modifying rom_vals while you're looping over it.

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