new output class and add configuration output in ".xyz" format #42
Conversation
|
My unit tests are failing in this branch -- looks like an error is triggered by a syntax error on line 319 of otf.py. Make sure you run pytest in the flare/tests directory to check that edits don't break the tests. |
|
@jonpvandermause all test ran well on my python environment on Odyssey... could you please show the syntax error? so I can figure out what's wrong? |
|
Turns out I was 12 commits behind. This was the offending line (after commit 20374f9, since been corrected): The tests work at the tip of the branch, but after running them the following files were generated and flagged by git after "git status": Should we update the .gitignore so that these aren't tracked? |
…ted log files after test in test_gp_from_aimd
|
The latest commit should also solve issue #37 . All the hyper-parameter optimization output is redirected to a file with "-hyps.dat" suffix. |
| @@ -9,7 +9,7 @@ | |||
| def run_espresso(qe_input, structure, pw_loc): | |||
| run_qe_path = qe_input | |||
| edit_qe_input_positions(run_qe_path, structure) | |||
| qe_command = 'mpirun {0} < {1} > {2}'.format(pw_loc, run_qe_path, | |||
| qe_command = '{0} < {1} > {2}'.format(pw_loc, run_qe_path, | |||
There was a problem hiding this comment.
This is a good catch, puts more power in the hand of the user to define the PWSCF command with/without MPI
| @@ -24,6 +24,8 @@ def run_espresso_par(qe_input, structure, pw_loc, no_cpus): | |||
| 'mpirun -np {0} {1} < {2} > {3}'.format(no_cpus, pw_loc, run_qe_path, | |||
| 'pwscf.out') | |||
|
|
|||
| with open("alog", "a+") as fout: | |||
There was a problem hiding this comment.
What does alog mean / stand for?
There was a problem hiding this comment.
ah... It's a line I left for debug. I forgot to delete it...
| print('like: ' + str(like)) | ||
| print('\n') | ||
| if output is not None: | ||
| output.write_to_log('like: ' + str(like)+'\n', name="hyps") |
There was a problem hiding this comment.
A suggestion which is somewhat to personal preference: I personally find formatting strings as
'like: {} \n'.format(like)
to be more readable.
I know that you also just translated the existing line this way, but going forward we could do all of our string formatting this way.
| @@ -244,11 +241,11 @@ def train_gp(self): | |||
| """ | |||
| Train the Gaussian process and write the results to the output file. | |||
| """ | |||
| self.gp.train(monitor=True if self.verbose >= 2 else False) | |||
| self.gp.train(output=self.output if self.verbose >= 2 else None) | |||
There was a problem hiding this comment.
I like the way you implemented this (passing output as the output object or None based on verbosity).
| @@ -24,7 +24,7 @@ def __init__(self, kernel: Callable, | |||
| energy_kernel: Callable = None, | |||
| opt_algorithm: str = 'L-BFGS-B', | |||
| maxiter=10, par=False, | |||
| monitor=False): | |||
| output=None): | |||
There was a problem hiding this comment.
Here and in a few other places where output is passed as an argument, we could typehint output as the Output object; in a few locations, we pass in an output location as a string (such as in the TrajectoryTrainer class). We should make sure it's typehinted there as well.
There was a problem hiding this comment.
Type hints will be eliminated eventually, as they don't play well with the Sphinx documentation. Might be better to do without them.
There was a problem hiding this comment.
(The type of the input should be made clear in the docstring anyway)
There was a problem hiding this comment.
Sad to hear that Sphinx can't handle them. That's a shame. At least docstrings can contain the same info.
There was a problem hiding this comment.
Actually it looks like Sphinx only really struggles with np.ndarray type hints. Here's an example from the docs:
If we can find a way to get Sphinx to format that better, type hints should be okay.
| """ | ||
| self.outfiles[name].write(logstring) | ||
|
|
||
| def write_header(self, cutoffs, kernel_name, |
There was a problem hiding this comment.
In the future, we should consider writing a more general function that takes a dictionary in as an argument and then prints the key, value pairs in a similar way that the header method does (as well as a few other methods in this document which are tailored to individual use cases).
There was a problem hiding this comment.
Thanks for taking the initiative on this, the output rewrite looks good. I appreciate that it is well-integrated with all of the existing output functionality. I have a few tiny quibbles: I would ask that you do a once-over at some point and add typehints to the new methods and functions and tighten up the docstrings. I think that making a general write_dict method is not strictly necessary for this PR but would be nice.
A new class Output is set to replace the original output module. The class opens a log file and two xyz files at the init function and closes all files in conclude_run() function. All the original functions in the output module now become function members in the Output class, with the same arguments.
new class for output and write atomic configurations to xyz files
The new class avoid open/close the log file too often and also automatically back up the old log file if it exists before the current run starts.
write_xyz_config(...) function is also added to print configs in xyz format. It is called in the record_state function in otf.py and md_run.py.
Completely: