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

Ver 1ka #183

Merged
merged 24 commits into from
Oct 14, 2024
Merged

Ver 1ka #183

merged 24 commits into from
Oct 14, 2024

Conversation

Lee01Atom
Copy link
Contributor

@Lee01Atom Lee01Atom commented Oct 1, 2024

(Ref. #12)

EDIT: for your next PR, @Lee01Atom, you should list the issue number here too, just like I added above.

@Lee01Atom Lee01Atom closed this Oct 1, 2024
@Lee01Atom Lee01Atom deleted the ver-1ka branch October 1, 2024 15:24
@Lee01Atom Lee01Atom restored the ver-1ka branch October 1, 2024 15:24
@Lee01Atom Lee01Atom reopened this Oct 1, 2024
@simopier simopier self-assigned this Oct 3, 2024
@simopier
Copy link
Collaborator

simopier commented Oct 3, 2024

@Lee01Atom see my emails on how to clean this up. If that doesn't work, let me know.

@simopier simopier added the V&V Relevant to V&V label Oct 4, 2024
@moosebuild
Copy link

moosebuild commented Oct 4, 2024

Job Documentation, step Sync to remote on b0b0668 wanted to post the following:

View the site here

This comment will be updated on new commits.

Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

A couple of comments (to add to the outstanding remaining comments in #178) to reduce the computational time of the test.

test/tests/ver-1ka/ver-1ka.i Outdated Show resolved Hide resolved
test/tests/ver-1ka/ver-1ka.i Outdated Show resolved Hide resolved
test/tests/ver-1ka/ver-1ka.i Outdated Show resolved Hide resolved
test/tests/ver-1ka/ver-1ka.i Outdated Show resolved Hide resolved
@moosebuild
Copy link

moosebuild commented Oct 4, 2024

Job Coverage, step Generate coverage on b0b0668 wanted to post the following:

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.

@Lee01Atom
Copy link
Contributor Author

@simopier the Test/Test Debug seems to work.

There is still an issue with the Documentation:
in the .md file, "comparison_ver-1ka.py" is not found, although it exists in doc/content/verification_and_validation/comparison_ver-1ka.py.
Could I be missing something in the automatically generated documentation?

Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

Great improvements, @Lee01Atom!

I left several comments throughout, and the main one for the documentation is the command for the alias.

Let me know if you have any questions.

Note to self: I will still need to review the documentation once it builds.

doc/content/verification_and_validation/index.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
test/tests/ver-1ka/comparison_ver-1ka.py Show resolved Hide resolved
test/tests/ver-1ka/tests Outdated Show resolved Hide resolved
test/tests/ver-1ka/tests Outdated Show resolved Hide resolved
test/tests/ver-1ka/tests Show resolved Hide resolved
@simopier
Copy link
Collaborator

@simopier the Test/Test Debug seems to work.

There is still an issue with the Documentation: in the .md file, "comparison_ver-1ka.py" is not found, although it exists in doc/content/verification_and_validation/comparison_ver-1ka.py. Could I be missing something in the automatically generated documentation?

See a suggestion in my latest review above. Let me know if you're running into issues.
By the way, to create the documentation locally, you can do:

conda activate moose
cd ~/projects/TMAP8/doc 
./moosedocs.py build --serve

Co-authored-by: Pierre-Clement Simon <[email protected]>
doc/content/verification_and_validation/index.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
test/tests/ver-1ka/ver-1ka.i Outdated Show resolved Hide resolved
Co-authored-by: Pierre-Clement Simon <[email protected]>
@simopier
Copy link
Collaborator

You are getting:

##########################################################################
ERROR: The following files contain trailing whitespace after applying your patch:
	test/tests/ver-1ka/ver-1ka.i

Run the "delete_trailing_whitespace.sh" script in your $MOOSE_DIR/scripts directory.
##########################################################################

Co-authored-by: Pierre-Clement Simon <[email protected]>
test/tests/ver-1ka/tests Outdated Show resolved Hide resolved
Co-authored-by: Pierre-Clement Simon <[email protected]>
Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

There is a typo in the python script

test/tests/ver-1ka/comparison_ver-1ka.py Outdated Show resolved Hide resolved
Co-authored-by: Pierre-Clement Simon <[email protected]>
Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

Some minor comments for the documentation

doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
doc/content/verification_and_validation/ver-1ka.md Outdated Show resolved Hide resolved
Co-authored-by: Pierre-Clement Simon <[email protected]>
Copy link
Collaborator

@simopier simopier left a comment

Choose a reason for hiding this comment

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

Some last suggestions (I should have added those earlier but I missed this), and then you're good to go!

test/tests/ver-1ka/comparison_ver-1ka.py Show resolved Hide resolved
test/tests/ver-1ka/comparison_ver-1ka.py Outdated Show resolved Hide resolved
test/tests/ver-1ka/comparison_ver-1ka.py Outdated Show resolved Hide resolved
Co-authored-by: Pierre-Clement Simon <[email protected]>
Copy link
Collaborator

@simopier simopier 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 and congratulations for your first contributions, @Lee01Atom!

@simopier simopier merged commit cb9bfbf into idaholab:devel Oct 14, 2024
8 checks passed
@simopier simopier mentioned this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V&V Relevant to V&V
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants