Skip to content

Mattdon/new melcor interface #2017

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

Open
wants to merge 50 commits into
base: devel
Choose a base branch
from
Open

Conversation

mandd
Copy link
Collaborator

@mandd mandd commented Nov 17, 2022


Pull Request Description

What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)

Closes #2125

What are the significant changes in functionality due to this change request?

For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass, including run_tests, pylint, manual building and xsd tests. If there are changes to Simulation.py or JobHandler.py the qsub tests must pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test. If new development on the internal JobHandler parallel system is performed, a cluster test must be added setting, in XML block, the node <internalParallel> to True.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added is the the analytic documentation updated/added?
  • 9. If any test used as a basis for documentation examples (currently found in raven/tests/framework/user_guide and raven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.

@moosebuild
Copy link

Job Mingw Test on 3eb7f67 : invalidated by @joshua-cogliati-inl

restarted civet

alfoa
alfoa previously approved these changes Apr 4, 2025
Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

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

@mandd I have a couple of comments for you to consider.

@@ -13,104 +13,168 @@
# limitations under the License.
"""
Created on April 18, 2017
@author: Matteo Donorio (University of Rome La Sapienza),
Fabio Gianneti (University of Rome La Sapienza),
@author: Matteo D'Onorio (Sapienza University of Rome)
Andrea Alfonsi (INL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably you need to update the affiliation here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +29 to +30
This class is used a part of a code dictionary to specialize Model. Code for different MELCOR versions
like MELCOR 2.2x, MELCOR 1.86, MELCOR for fusion applications
Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, is there any way that we can check the MELCOR version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure tbh

from .melgenInterface import MelgenApp
from ..Generic import GenericParser
import pandas as pd


class Melcor(CodeInterfaceBase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make Melcor interface inherit from GenericCodeInterface? I saw you are using GenericParser already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we rely only on the genericParser which is a separate module also called by GenericCode interface

Comment on lines +112 to +113
Two addtional nodes need to be specified: the first one (i.e., \xmlNode{CodePlotFile}) indicates the name of the MELCOR plot file.
The second one (i.e., \xmlNode{MelcorOutput}) specifies the name of the MELCOR message file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest add more detailed descriptions here. For example, what are the text format for the nodes? I assume the code can only accept one file for each of the nodes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added a small additional description, now that we have a MELCOR user in the group we will gather more experience on this interface

Comment on lines 55 to 56
self.melcorPlotFile = [var.strip() for var in plotNode.text.split(",")][0]
self.melcorOutFile = [var.strip() for var in melNode.text.split(",")][0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines indicate you may allow users to provide more than one files, but you only use the first file. Could you clarify why you would like to do it in this way? Do you expect to extend the interface to accept more files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

returnCommand = melgCommand + melcCommand, melcOut
found = False

melcOut = 'OUTPUT_MELCOR'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest you define the "melcOut" in the init method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

precommand = executable
melgCommand = str(preExec) + ' ' + melcin.getFilename()
melcCommand = str(precommand) + ' ' + melcin.getFilename()
returnCommand = [('serial',melgCommand + ' && ' + melcCommand +' ow=o ')],melcOut
Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, can Melcor run in parallel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i do not think so

Comment on lines +127 to +129
parser = GenericParser.GenericParser(inFiles)
parser.modifyInternalDictionary(**Kwargs)
parser.writeNewInput(currentInputFiles,origFiles)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me you can make this class inherit from Generic Code Interface class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see comment above

failure = self.melcorInterface.checkForOutputFailure(output, workingDir)
return failure
failure = True
goodWord = "Normal termination" # This is for MELCOR 2.2 (todo: list for other MELCOR versions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest we define the "goodWord" in init method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved to init

@moosebuild
Copy link

Job Test mac on de9ceac : invalidated by @joshua-cogliati-inl

failed in set python environment with download failure

@alfoa
Copy link
Collaborator

alfoa commented Apr 17, 2025

@mandd I would also move the MELCOR test(s) in its dedicated folder (for better organization of the CodeInterface test folder)

@moosebuild
Copy link

Job Mingw Test on 1e8fbe9 : invalidated by @joshua-cogliati-inl

restarted civet

Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

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

@mandd Some minor comments for you to consider.

Comment on lines 6 to 7
<author>mattdon</author>
<created>2023-02-20</created>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should not change the author here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

def finalizeCodeOutput(self, command, output, workingDir):
if "dynamicevent" in samplerType.lower():
raise IOError("Dynamic Event Tree-based samplers not implemented for MELCOR yet! But we are working on that.")
indexes = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems variable "indexes" is not used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

Copy link
Collaborator Author

@mandd mandd left a comment

Choose a reason for hiding this comment

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

addressing latest comments

Comment on lines 6 to 7
<author>mattdon</author>
<created>2023-02-20</created>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@moosebuild
Copy link

All jobs on eeebb8c : invalidated by @wangcj05

timeout and fetch issue

@moosebuild
Copy link

Job Test Ubuntu 18 PIP on eeebb8c : invalidated by @mandd

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.

[TASK] MELCOR interface
6 participants