Renamed many classes. - #90
Conversation
e45a4f3 to
49ccb5a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR renames many classes throughout the codebase to better reflect their purpose and usage patterns. The primary changes focus on improving clarity by renaming "Program" prefixed classes to "Calc" (calculation) prefixed classes, and "Results" to "Data" for attributes that represent computed values.
Key changes include:
- Renamed core classes like
ProgramOutput→Results,ProgramInput→CalcInput - Updated attribute names like
.results→.data,.stdout→.logs - Added backwards compatibility shims with deprecation warnings
- Updated documentation and visualization code to use new naming
Reviewed Changes
Copilot reviewed 37 out of 39 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_view.py | Updated imports and variable names to use new class names |
| tests/test_serialization.py | Changed parameter names and class references |
| tests/test_results.py | New file with tests for renamed Results class and data structures |
| tests/test_qcel.py | Updated parameter names to use new naming convention |
| tests/test_outputs.py | File removed (tests moved to test_results.py) |
| tests/test_optimization_data.py | Updated to use new OptimizationData class name |
| tests/test_conformer_search_data.py | Updated to use new ConformerSearchData class name |
| tests/test_calc_input.py | Updated to use new CalcInput class name |
| tests/conftest.py | Updated fixture names and class references |
| src/qcio/view.py | Updated class names in visualization code |
| src/qcio/qcel.py | Updated import and function parameter types |
| src/qcio/models/structure.py | Updated base class name |
| src/qcio/models/results.py | Major refactor with class renames and compatibility shims |
| src/qcio/models/inputs.py | Updated class names with compatibility aliases |
| src/qcio/models/base_models.py | Renamed QCIOModelBase to QCIOBaseModel |
| src/qcio/models/init.py | Updated import reference |
| mkdocs.yml | Updated documentation structure and references |
| docs/... | Updated documentation files to reflect new naming |
Comments suppressed due to low confidence (2)
tests/test_results.py:1
- Duplicate
Resultsclass in the list. Based on the TODO comment above, this should likely be[Results, ProgramOutput]to register both the new and backwards compatibility classes.
import pickle
src/qcio/view.py:718
- Should use
po.datainstead ofpo.resultsto access the data attribute directly rather than through the deprecated backwards compatibility property.
results_html = generate_results_table(po.results)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| assert deserialized.data == results.results | ||
| assert deserialized.input_data == results.input_data | ||
| assert deserialized.provenance.program == "qcio-test-suite" | ||
| assert deserialized.logs == results.logs | ||
| assert deserialized.extras == results.extras | ||
| # assert deserialized.return_result == prog_output.return_result | ||
| assert deserialized.data.energy == results.results.energy | ||
| assert np.array_equal(deserialized.data.gradient, results.results.gradient) | ||
| assert np.array_equal(deserialized.data.hessian, results.results.hessian) |
There was a problem hiding this comment.
Inconsistent attribute access: deserialized.data should be compared to results.data, not results.results. The .results attribute is the deprecated backwards compatibility property.
| assert deserialized.data == results.results | |
| assert deserialized.input_data == results.input_data | |
| assert deserialized.provenance.program == "qcio-test-suite" | |
| assert deserialized.logs == results.logs | |
| assert deserialized.extras == results.extras | |
| # assert deserialized.return_result == prog_output.return_result | |
| assert deserialized.data.energy == results.results.energy | |
| assert np.array_equal(deserialized.data.gradient, results.results.gradient) | |
| assert np.array_equal(deserialized.data.hessian, results.results.hessian) | |
| assert deserialized.data == results.data | |
| assert deserialized.input_data == results.input_data | |
| assert deserialized.provenance.program == "qcio-test-suite" | |
| assert deserialized.logs == results.logs | |
| assert deserialized.extras == results.extras | |
| # assert deserialized.return_result == prog_output.return_result | |
| assert deserialized.data.energy == results.data.energy | |
| assert np.array_equal(deserialized.data.gradient, results.data.gradient) | |
| assert np.array_equal(deserialized.data.hessian, results.data.hessian) |
| assert deserialized.provenance.program == "qcio-test-suite" | ||
| assert deserialized.logs == results.logs | ||
| assert deserialized.extras == results.extras | ||
| # assert deserialized.return_result == prog_output.return_result |
There was a problem hiding this comment.
This commented-out assertion should either be uncommented and fixed to use the new variable names, or removed entirely if it's no longer needed.
| # assert deserialized.return_result == prog_output.return_result |
| final_html.append(generate_output_table(po)) | ||
|
|
||
| if isinstance(po.results, ConformerSearchResults): | ||
| if isinstance(po.results, ConformerSearchData): |
There was a problem hiding this comment.
Should use po.data instead of po.results since the Results class attribute was renamed from .results to .data. The .results property exists for backwards compatibility but emits deprecation warnings.
|
|
||
| # Determine the Structure to use | ||
| if isinstance(po.results, OptimizationResults): | ||
| if isinstance(po.results, OptimizationData): |
There was a problem hiding this comment.
Multiple instances where po.results should be po.data to use the new attribute name instead of the deprecated backwards compatibility property.
| title_extra += " (Final Structure)" | ||
|
|
||
| elif isinstance(po.results, SinglePointResults): | ||
| elif isinstance(po.results, SinglePointData): |
There was a problem hiding this comment.
Multiple instances where po.results should be po.data to use the new attribute name instead of the deprecated backwards compatibility property.
|
|
||
| # Create results table or plot | ||
| if isinstance(po.results, OptimizationResults): | ||
| if isinstance(po.results, OptimizationData): |
There was a problem hiding this comment.
Multiple instances where po.results should be po.data to use the new attribute name instead of the deprecated backwards compatibility property.
| "source": [ | ||
| "# Inspect a single frame. This will be ProgramOutput for a single point calculation\n", | ||
| "# Inspect a single frame. This will be Results for a single point calculation\n", | ||
| "view.view(xtb_opt.results.trajectory[-1], width=400, height=350)" |
There was a problem hiding this comment.
Should use xtb_opt.data.trajectory[-1] instead of xtb_opt.results.trajectory[-1] to access the data attribute directly rather than through the deprecated backwards compatibility property.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/qcio/view.py:457
- Should use the new
.dataattribute instead of the deprecated.resultsattribute for consistency with the renaming.
energies = prog_output.results.energies * constants.HARTREE_TO_KCAL_PER_MOL
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
b3601a8 to
5387b62
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import numpy as np | ||
|
|
||
| from qcio import ProgramInput, SinglePointResults, Wavefunction | ||
| from qcio import CalcSpec, SinglePointResults, Wavefunction |
There was a problem hiding this comment.
Import still references the old class name SinglePointResults instead of the new SinglePointData.
| setattr(this_module, name, _class) | ||
| for input_type, results_type in product(get_args(Specs), get_args(Data)): | ||
| # TODO: Remove ProgramOutput when compatibility is no longer needed | ||
| for ClassType in [Results, Results]: |
There was a problem hiding this comment.
The loop iterates over [Results, Results] (duplicated), but the comment suggests it should include ProgramOutput for backward compatibility.
| for ClassType in [Results, Results]: | |
| for ClassType in [Results, ProgramOutput]: |
| @deprecated_class("CoreSpec") | ||
| class ProgramArgs(CoreSpec): | ||
| """Deprecated alias for CalcArgs. | ||
|
|
||
| This class is deprecated and will be removed in a future release. Please use | ||
| `CoreSpec` instead. | ||
| """ |
There was a problem hiding this comment.
Corrected class name in docstring from 'CalcArgs' to 'CoreSpec'.
| model: Optional[Model] = None | ||
| subprogram: str | ||
| subprogram_args: ProgramArgs | ||
| subprogram_spec: CoreSpec |
There was a problem hiding this comment.
The docstring still refers to 'ProgramArgs' instead of 'CoreSpec' for the subprogram_spec attribute type.
| - [`Structure`](./structure.md) objects represent a collection of atoms, a molecule, or any super molecular structure in 3D cartesian space. | ||
| - [`Input`](./inputs.md) objects define the input parameters for a calculation. | ||
| - [`ProgramOutput`](./outputs.md) objects store computed values and output files (collectively called [`Results`](./outputs.md#qcio.Results)) from a calculation. `ProgramOutput` also stores the exact input data (`.input_data`) used for a calculation, relevant metadata, and [`Provenance`](./provenance.md) information so you have full visibility of how every result was generated. | ||
| - [`Calculation Specification`](./calculation_specificaions.md) objects define the parameters for a calculation. |
There was a problem hiding this comment.
Corrected spelling of 'specifications' in the link path.
| - [`Calculation Specification`](./calculation_specificaions.md) objects define the parameters for a calculation. | |
| - [`Calculation Specification`](./calculation_specifications.md) objects define the parameters for a calculation. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 40 out of 42 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/qcio/view.py:1
- [nitpick] Variable name
po(likely short for 'program output') should be updated toresultto match the new naming convention where the class is now calledResults.
"""
src/qcio/view.py:435
- The code is accessing
.subprogram_argswhich has been renamed to.subprogram_spec. This should bepo.input_data.subprogram_spec.modelandpo.input_data.subprogram_spec.keywords.
if isinstance(po.input_data, CompositeCalcSpec):
base_row += f"""
<td>{po.input_data.subprogram}</td>
<td>{po.input_data.subprogram_args.model}</td>
<td>{generate_dictionary_string(po.input_data.subprogram_args.keywords)}</td>
"""
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| @deprecated_class("OptimizationData") | ||
| class OptimizationResults(OptimizationData): | ||
| """ "This class is deprecated and will be removed in a future release. Please use |
There was a problem hiding this comment.
Removed extra quote character at the beginning of the docstring.
| """ "This class is deprecated and will be removed in a future release. Please use | |
| """This class is deprecated and will be removed in a future release. Please use |
| assert opt_res.energies == [results.results.energy] | ||
| assert opt_res.structures == [results.input_data.structure] |
There was a problem hiding this comment.
The code is accessing .results attribute which has been renamed to .data. This should be results.data.energy to match the new naming convention.
2835623 to
6511172
Compare
6511172 to
380bff4
Compare
No description provided.