-
Notifications
You must be signed in to change notification settings - Fork 135
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
Serpent Interface Upgrade #2290
Conversation
…test machine centos8 (since by default it stills uses python 2 for initial installation of plugins
...framework/CodeInterfaceClasses/SERPENT/utilityForCreatingVariables/generateRavenVariables.py
Outdated
Show resolved
Hide resolved
…Variables/generateRavenVariables.py
ravenframework/CodeInterfaceClasses/SERPENT/SerpentInterface.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/SerpentInterface.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
ravenframework/CodeInterfaceClasses/SERPENT/serpentOutputParser.py
Outdated
Show resolved
Hide resolved
fixed some exceptions
…n into alfoa/serpentModifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alfoa I have several general comments for you to consider.
self._fileTypesToRead = ['ResultsReader'] # container of file types to read | ||
# in case of burnup calc, the interface can compute the time at which FOMs (e.g. keff) crosses | ||
# a target. For example (default), we can compute the time (burnDays) at which absKeff crosses 1.0 | ||
self.EOLtarget = {'absKeff':1.0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind do not like the naming here, but I do not have a better suggestion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe eolTarget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. EOL is the acronym for "End Of Life". I changed it following your suggestion above.
#if 'set mdep' not in data: | ||
# raise Exception("MicroXSReader file type has been requested but no 'mdep' flag has been set in the input file!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to keep these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would like to keep those lines because are "ready" to be used once a "defect" in serpentTools is addressed
#if 'set his' not in data: | ||
# raise Exception("HistoryReader file type has been requested but no 'his' flag has been set in the input file!") | ||
#else: | ||
# optionSet = data.split('set his')[1].strip() | ||
# if not optionSet.startswith('1'): | ||
# raise Exception("HistoryReader file type has been requested but 'set his' flag is not set to '1'!") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to keep these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would like to keep those lines because are "ready" to be used once a "defect" in serpentTools is addressed
#elif ft == 'HistoryReader': | ||
# results.update(self._historyReader(nSteps)) | ||
#elif ft == 'MicroXSReader': | ||
# results.update(self._microXSReader(nSteps)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to keep these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would like to keep those lines because are "ready" to be used once a "defect" in serpentTools is addressed
@ Out, None | ||
""" | ||
if 'keff' in k.lower() and k.lower() != 'anakeff': | ||
rho_sigma, rhoLog_sigma = None, None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelBack for these variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
rho, rhoLog = (v[0] - 1) / v[0], np.log(v[0]) | ||
if v.shape[0] > 1: | ||
# we have sigma | ||
rho_sigma, rhoLog_sigma = (v[1] / v[0]) * rho, (v[1] / v[0]) * rhoLog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
camelBack for these variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,62 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add copyright info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
try: | ||
sigmaAdjusted = calculations['sigma'][list(needed[metric]['targets'])]/np.sqrt(calculations['equivalentSamples'][list(needed[metric]['targets'])]) | ||
except: | ||
sigmaAdjusted = calculations['sigma'][list(needed[metric]['targets'])]/np.sqrt(self.sampleSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put the error type in the "except"? it seems to me these changes may not be the right fix for the issue. I guess equivalentSamples may not computed for certain cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the error type. What the right fix you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess maybe we need to figure why equivalentSamples is not calculated in certain case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equivalentSamples is not computed when self.pbPresent
is False.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to assign uniform weight in this case, reset self.pbPressent, and raise a warning for user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you guys do it in another MR? It is not really related to this MR. I can revert the fix if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely, you do not need to revert the changes. By the way, I have already approved your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful.
Thanks a lot!!!
def importOrInstall(package): | ||
""" | ||
Method to import or install (if not found) a library named "package" | ||
@ In, package, str, the package name to import | ||
@ Out, pckImport, module, the imported module | ||
|
||
NOTE: This method should be used only by developers | ||
since it silently install packages | ||
if not found | ||
""" | ||
pckImport = None | ||
try: | ||
pckImport = __import__(package) | ||
except ImportError: | ||
print("( ) "+UreturnPrintTag('UTILS')+": "+UreturnPrintPostTag('Message') + " -> Python package "+package+"not found. Trying to install it via pip!") | ||
import subprocess | ||
s = subprocess.getstatusoutput("python -m pip install " + package) | ||
if int(s[0]) == 0: | ||
print("( ) "+ UreturnPrintTag('UTILS')+": "+ UreturnPrintPostTag('Message') + " -> Installation succeded!") | ||
pckImport = __import__(package) | ||
else: | ||
print("( ) " + UreturnPrintTag('UTILS')+ ": " + UreturnPrintPostTag('Message') + " -> Installation failed with error: " + s[1] ) | ||
return pckImport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated, this method should be used only by developers. Can you add the package through RAVEN dependencies file? You may use optional attribute if you do not want to force to install it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. And also modified the test to check for that library (if not found, the tests should be skipped)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wangcj05 I don't remember. Are the "optional" libraries installed in any machine? (If not, the serpent regression tests are never run)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have one unix machine to test optional, however, for your situation, the library serpentTools is not installed. It seems something is wrong with the scripts/library_handler.py, the optional library is not added to pip
for some reason. I would suggest you update the script to handle the pip with optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that serpentTools is now installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes are good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes are good.
Job Test qsubs sawtooth on a5b2cd7 : invalidated by @wangcj05 very slow in removing packages |
Job Test qsubs sawtooth on a5b2cd7 : invalidated by @wangcj05 library issue |
checklist passed. PR can be merged. |
@wangcj05 Thanks a lot!!! |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Closes #2289
What are the significant changes in functionality due to this change request?
This merge request is aimed to upgrade and make the SERPENT interface fully functional and flexible.
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>
to True.raven/tests/framework/user_guide
andraven/docs/workshop
) have been changed, the associated documentation must be reviewed and assured the text matches the example.