Open maxentile opened 4 years ago
atom_k
and atom_eq
should be renamed to epsilon
and sigma
or similar.atom_k and atom_eq should be renamed to epsilon and sigma or similar.
this was just my shorthand to enable us to loop through terms like:
for term in ['atom', 'angle', 'bond']:
for param in ['eq', 'k']:
Yeah, noticed that in a few places, would need to refactor slightly so we have something like parameter_names['atom'] = ['sigma', 'epsilon', 'charge']
, parameter_names['torsion'] = ['periodicities', 'force_constants', 'phase_offsets']
, ..., or similar, and then say
for term in ['atom', 'angle', 'bond', 'torsion']:
for param in parameter_names[term]:
let's port some of the report-generating schemes that I implemented here
Nice! Looks like something in this direction may be an improvement: would separate the computation of summary statistics from the generation of formatted reports, which are currently intertwined.
A couple minor comments:
results
dictionaries of specific structure that depends on the result type, hinting that these may be better to live inside a results class (results.save_html()
, multiple_results_object.save_html()
, multiple_results_object.save_html(grid=True)
, ..., rather than html(results_dict)
, html_multiple_train_and_test(results)
, html_multiple_train_and_test_2d_grid(results)
...)
The report generators in
supervised_train.py
andsupervised_param_train.py
are great! They make it much easier to browse results of the numerical experiments @yuanqing-wang has been doing.A wishlist for things that would be good to include in the future iterations of the report generator:
loss_fn=mse_loss
, but @yuanqing-wang mentions by Slack that this loss is measured on a normalized regression target.1 - (residual sum of squares) / (total sum of squares)
, as in sklearn.metric.r2_score, but a reader might reasonably expect one of the other definitions that leads to a non-negative value.