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

Saltproc capability addition and UI improvement #29

Merged
merged 110 commits into from
Oct 9, 2018

Conversation

jbae11
Copy link
Contributor

@jbae11 jbae11 commented Aug 18, 2018

This PR includes addition of the following capabilities:

  1. user-input reprocessing scheme (solves User-specified input stream #19)
  2. Two-region simulation
  3. input file (run_saltproc.py) to drive saltproc run (solves Look for file paths in the environment and input arguments #27, Look for serpent executible in the environment and input arguments  #28 )
  4. more unit tests
  5. 'Smarter' looking up isotopes from the xs library (solves Generic input file for element specifying #18)
  6. Continuous integration (solves Set up continuous integration for salt proc #26)
  7. Saving runinfo (user input parameters) into db (solves save runinfo to database: #24)
  8. conversion from .bumat to .dep file (solves move from bumat -> .dep file #22 )

@pep8speaks
Copy link
Contributor

Here you go with the Pull Request ! The fixes are suggested by autopep8.

@jbae11

@jbae11 jbae11 changed the title Combined Saltproc capability addition and UI improvement Aug 18, 2018
if key == self.driver_mat_name:
key = 'driver'
elif key == self.blanket_mat_name:
key = 'blanket'
Copy link
Member

Choose a reason for hiding this comment

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

Why are these four lines copy pasted from above? Maybe they should be their own function.

Never copy paste. Use the DRY principle (Don't Repeat Yourself)


if restart:
# resize dataset
Copy link
Member

Choose a reason for hiding this comment

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

If it needs a comment to be readable, then this should probably be its own function.

@@ -374,6 +383,7 @@ def reopen_db(self, restart):
# set past time
# !! this time thing should be made certain
self.current_step = np.amax(np.nonzero(self.keff)) + 1
self.current_step = np.amax(np.nonzero(sum(self.driver_before_db))) + 1
Copy link
Member

Choose a reason for hiding this comment

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

Here you're setting a variable equal to something and then set it equal to something else in the next line.
Delete one of the two. Ideally, simplifying the syntax.

@@ -392,8 +402,11 @@ def reopen_db(self, restart):

# write new material file
self.core = {}
self.core[self.driver_mat_name] = self.driver_after_db[self.current_step - 1]
self.core[self.blanket_mat_name] = self.blanket_after_db[self.current_step - 1]
self.core[self.driver_mat_name] = self.driver_after_db[self.current_step - 2]
Copy link
Member

Choose a reason for hiding this comment

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

why hardcode this number 2?

self.core[self.blanket_mat_name] = self.blanket_after_db[self.current_step - 1]
self.core[self.driver_mat_name] = self.driver_after_db[self.current_step - 2]
self.core[self.blanket_mat_name] = self.blanket_after_db[self.current_step - 2]
print(self.current_step - 1)
Copy link
Member

Choose a reason for hiding this comment

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

why print this?

self.core[self.blanket_mat_name] = self.blanket_after_db[self.current_step - 2]
print(self.current_step - 1)
print(self.core[self.driver_mat_name])
print(self.core[self.blanket_mat_name])
Copy link
Member

Choose a reason for hiding this comment

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

Are these print statements for debugging or runtime logging? If the former, I recommend pdb. If the latter, I recommend adding a logging system and .log file output to saltproc.

@@ -461,13 +474,13 @@ def read_dep(self, boc=False):
indx = z.index('%')
mdens = z[indx-1]
if boc:
mdens = z[indx-2]
mdens = z[0]
Copy link
Member

Choose a reason for hiding this comment

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

why?

@@ -493,8 +506,9 @@ def write_mat_file(self):
(self.current_step, ana_keff_boc[0], ana_keff_boc[1],
ana_keff_eoc[0], ana_keff_eoc[1]))
for key, val in self.core.items():
matf.write(self.mat_def_dict[key].replace(
'\n', '') + ' fix 09c 900\n')
if key == '':
Copy link
Member

Choose a reason for hiding this comment

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

What if key was never initialized? It might be a None type.

# False: Start from timestep zero.
restart = False

# True: Uses blue water command (aprun) to run SERPERNT
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean "Blue Waters"

##############################################################

# SERPENT input file
input_path = os.path.dirname(os.path.abspath(__file__))
Copy link
Member

Choose a reason for hiding this comment

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

This line will confuse the user. It shouldn't be edited, but it's the first line in the section that is supposed to be for setting variables.

input_path = os.path.dirname(os.path.abspath(__file__))
sys.path.append(input_path)
print(input_path)
input_file = os.path.join(input_path, 'mcsfr_design3.inp')
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious to the user what they should and shouldn't edit.
If you're going to do this input file thing, it should only be variables that the user can customize, nothing else. It means doing the path join later in the input parsing step, but it will keep your input file clean.

# material file with fuel composition and density
mat_file = os.path.join(input_path, 'iter_mat_file')

init_mat_file = os.path.join(input_path, 'mat_composition3.inp')
Copy link
Member

Choose a reason for hiding this comment

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

This one deserves a description just like the others have.

# executable path of Serpent
exec_path = '/projects/sciteam/bahg/serpent30/src/sss2'

# Number of cores and nodes to use in cluster
Copy link
Member

Choose a reason for hiding this comment

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

in the cluster


Copyright (c) 2015--, Ariel Rokem, The University of Washington
Copy link
Member

Choose a reason for hiding this comment

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

We should keep a license file called LICENSE_shablona which credits ariel for the shablona backbone of this work. THat's the point of an attirbution license. You don't just delete it.

"""

NAME = "saltproc"
MAINTAINER = "Ariel Rokem"
MAINTAINER_EMAIL = "arokem@gmail.com"
MAINTAINER = "Jin Whan Bae"
Copy link
Member

Choose a reason for hiding this comment

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

I think either Andrei or arfc is the maintainer.

MAINTAINER = "Ariel Rokem"
MAINTAINER_EMAIL = "arokem@gmail.com"
MAINTAINER = "Jin Whan Bae"
MAINTAINER_EMAIL = "jbae11@illinois.edu"
Copy link
Member

Choose a reason for hiding this comment

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

For the long term, arfc@googlegroups.com is best.

MAINTAINER = "Ariel Rokem"
MAINTAINER_EMAIL = "arokem@gmail.com"
MAINTAINER = "Jin Whan Bae"
MAINTAINER_EMAIL = "jbae11@illinois.edu"
DESCRIPTION = description
Copy link
Member

Choose a reason for hiding this comment

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

need description

AUTHOR = "Ariel Rokem"
AUTHOR_EMAIL = "arokem@gmail.com"
LICENSE = "BSD-3"
AUTHOR = "Andrei Rykhlevskii"
Copy link
Member

Choose a reason for hiding this comment

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

can have a list of people.

@jbae11
Copy link
Contributor Author

jbae11 commented Aug 29, 2018

@katyhuff Thank you for your review. I have reflected the smaller comments and made issues regarding larger (splitting into smaller functions (#31), Geometry-agnostic format (#30), More tests (#21)) edits. I feel like it's appropriate that these issues get their own PRs. What do you think? Thanks!

@jbae11
Copy link
Contributor Author

jbae11 commented Aug 30, 2018

#31 is addressed in the last 4 commits.

Copy link
Contributor

@andrewryh andrewryh left a comment

Choose a reason for hiding this comment

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

Merge before start refactoring

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.

4 participants