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

Introduce get_thermal_expansion_output() function #183

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

jan-janssen
Copy link
Member

@jan-janssen jan-janssen commented Jan 8, 2024

Remove the code duplication of defining multiple instances of the ThermalExpansionOutput by defining one function to create the corresponding output object:

def get_thermal_expansion_output(temperatures_lst, volumes_lst, output_keys):
    return OutputThermalExpansion(
        **{
            k: getattr(ThermalExpansionProperties, k)
            for k in OutputThermalExpansion.fields()
        }
    ).get(
        ThermalExpansionProperties(
            temperatures_lst=temperatures_lst, volumes_lst=volumes_lst
        ),
        *output_keys,
    )

EDIT: syntax highlighting

Again this change was initially introduced in #176 and is now moved to a separate pull request.

@jan-janssen jan-janssen added the format_black Launch the pyiron/actions black formatting routine label Jan 8, 2024
@jan-janssen jan-janssen assigned liamhuber and unassigned liamhuber Jan 8, 2024
@jan-janssen jan-janssen requested a review from liamhuber January 8, 2024 18:51
Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

One can do this, and the reduction of code duplication is indeed helpful, but I am sad that both OutputThermalExpansionProperties and OutputThermalExpansion both exist. The underlying symptom is that we're shoe-horning in an intermediate engine where no engine is necessary, because the dataclasses+callable infrastructure was conceived only in the context of lammps/ase/etc engines holding data. Still, might as well merge this as it at least makes the bad thing better.

…_output

# Conflicts:
#	atomistics/calculators/ase.py
#	atomistics/calculators/lammps/calculator.py
#	atomistics/calculators/lammps/helpers.py
#	atomistics/shared/thermal_expansion.py
@jan-janssen
Copy link
Member Author

..., but I am sad that both OutputThermalExpansionProperties and OutputThermalExpansion both exist.

I agree, as discussed in the meeting today, I am open for suggestions, I just want to merge the parts we can agree on, to simplify future discussions.

... Still, might as well merge this as it at least makes the bad thing better.

Ok.

@jan-janssen jan-janssen merged commit 5ddb9f6 into main Jan 8, 2024
19 checks passed
@jan-janssen jan-janssen deleted the get_thermal_expansion_output branch January 8, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black Launch the pyiron/actions black formatting routine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants