-
Notifications
You must be signed in to change notification settings - Fork 11
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
Feature/pacemaker2022 #417
Conversation
- add elements and cutoff properties - set default self.input - rewrite _save_structure_dataframe_pckl_gzip - write_input: if _train_job_id_list is non empty, after adding job.add_training_data(training_container), then compose training dataframe using 'job.create_training_dataframe' - automatically determine the list of elements if self.structure_data is pd.DataFrame - implement _get_training_data and _get_predicted_data
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@pmrv would you be able to review this quickly as we can then merge and release for the workshop. |
Pull Request Test Coverage Report for Build 2424052605
💛 - Coveralls |
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.
_get_training_data
and _get_predicted_data
should be fixed, otherwise things are ok. I'll change the input to DataContainer
after this is merged and then we can make a new release.
# set loggers | ||
loggers = [logging.getLogger(name) for name in logging.root.manager.loggerDict] | ||
for logger in loggers: | ||
logger.setLevel(logging.WARNING) |
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.
Is that necessary to hide some annoying warnings? Loggers are already setup and accessible via job.logger
. The logger has different handlers attached that filter by different levels for different outputs, so it should not be necessary to configure them again.
|
||
self._train_job_id_list = [] | ||
|
||
self.input = GenericParameters(table_name="input") |
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.
Should use DataContainer
, but I can also do that once this is merged.
def _get_training_data(self) -> TrainingStorage: | ||
# TODO: convert to TrainingStorage ? | ||
fname = os.path.join(self.working_directory, "fitting_data_info.pckl.gzip") | ||
df = pd.read_pickle(fname, compression="gzip") | ||
return df | ||
|
||
def _get_predicted_data(self) -> FlattenedStorage: | ||
# TODO: convert to FlattenedStorage ? | ||
fname = os.path.join(self.working_directory, "train_pred.pckl.gzip") | ||
df = pd.read_pickle(fname, compression="gzip") | ||
return df |
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.
If this is not implemented properly, the plotting functions will break. Ideally the job would keep a TrainingStorage
around and extend it every time _add_training_data
is called. For the workshop we can probably get away with something like this
df["atoms"] = df.ase_atoms.map(ase_to_pyiron)
t = TrainingStorage()
for _, r in d.iterrows():
t.add_structure(r.atoms, energy=r.energy_corrected, forces=r.forces, identifier=r['name'])
and something similar for _get_predicted_data
. If it contains the structures also use a TrainingStorage
otherwise use a FlattenedStorage
.
@yury-lysogorskiy could you please fix the issues as soon as possible. We can then release and get everything ready. thanks! |
extract training_data(TrainingStorage) and predicted_data_fs(FlattenedStorage) in collect_output
The example notebook blocks the CI, since it won't run without training data, so I've removed it for now. It's a very nice notebook though, so we should find a different place for it. I also realized that I do not have a possibility to test pacemaker jobs on our cluster yet, so I won't change the input to I've made issues #419 and #420 to remind me to pick it up after the workshop. |
@pmrv @yury-lysogorskiy Thanks for the work on this, I will merge it and release. |
No description provided.