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

That's not shared #169

Open
liamhuber opened this issue Jan 2, 2024 · 2 comments
Open

That's not shared #169

liamhuber opened this issue Jan 2, 2024 · 2 comments
Assignees

Comments

@liamhuber
Copy link
Member

Right now atomistics.output.shared has all sorts of classes in it that are not, nor are likely to ever be, shared. E.g., I wouldn't reasonably expect OutputEnergyVolumeCurve to ever be used outside of, unsurprisingly, the workflows.ev_curve submodule.

Either these Output classes should be moved down into their respective domains (we can promote them up to shared again some day if they ever become shared), or the atomistics.shared needs to be renamed, or atomistics.shared.output needs to be moved.

In #166 I went with the first choice and just moved the output class down into the workflow submodule.

@jan-janssen
Copy link
Member

The intention of having all data classes defined in atomistics.output.shared is to specify the ontology of the physical properties independent of the protocols to calculate those. For example currently the Energy Volume Curve workflow and the Elastic Matrix workflow both calculate the bulk modulus, so it would be great to represent this connection in the output class. But we have to be careful as the Elastic Matrix workflow does not calculate the equilibrium bulk modulus unless the input structure is already the equilibrium structure. As discussed in #157

@liamhuber
Copy link
Member Author

That's a perfectly reasonable objective, but unless and until these classes are actually shared it still makes it very confusing that the live at atomistics.shared.output. If the state of their shared-ness is going to change soon and/or often, then sliding the individual classes around is probably not the best choice. What about either moving the output file or renaming shared? Except for the tqdm stuff, it looks like something like physics would be a reasonable name.

My core objection is just that we should avoid module names that are provably false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants