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

Drop output_ prefix in output ports #950

Open
mbercx opened this issue Jun 3, 2023 · 2 comments
Open

Drop output_ prefix in output ports #950

mbercx opened this issue Jun 3, 2023 · 2 comments

Comments

@mbercx
Copy link
Member

mbercx commented Jun 3, 2023

Currently a lot of the outputs have output_ as a prefix in their port names, e.g.:

spec.output('output_parameters', valid_type=orm.Dict,
help='The `output_parameters` output node of the successful calculation.')
spec.output('output_structure', valid_type=orm.StructureData, required=False,
help='The `output_structure` output node of the successful calculation if present.')
spec.output('output_trajectory', valid_type=orm.TrajectoryData, required=False)
spec.output('output_band', valid_type=orm.BandsData, required=False,
help='The `output_band` output node of the successful calculation if present.')
spec.output('output_kpoints', valid_type=orm.KpointsData, required=False)
spec.output('output_atomic_occupations', valid_type=orm.Dict, required=False)
spec.default_output_node = 'output_parameters'

This is a bit redundant. It means we have to type e.g.:

pw_calc.outputs.output_parameters

Instead of just having

pw_calc.outputs.parameters

I think the second is:

  1. More intuitive.
  2. Shorter and hence more readable. It's still perfectly clear but there is no distracting duplicate ouput.
  3. Faster to tab complete. I can just do pw_calc.p+ Tab and get the output I want.

So I vote we remove all these output_ prefixes everywhere. Clearly this would be a backwards-incompatible change. @sphuber do you see an elegant deprecation pathway, e.g. keeping both output ports and raising a warning if the deprecated one is accessed?

@mbercx mbercx changed the title PwCalculation: Drop output_ prefix in output ports Drop output_ prefix in output ports Jun 3, 2023
@mbercx mbercx added this to the v5.0.0 milestone Jun 3, 2023
@sphuber
Copy link
Contributor

sphuber commented Jun 3, 2023

Conceptually, I am fully onboard with this. I have never liked the output_ prefix and even considered removing it way back when, but didn't, because the plugin was the most used and a lot of code would break.

@sphuber do you see an elegant deprecation pathway, e.g. keeping both output ports and raising a warning if the deprecated one is accessed?

I think it is going to be pretty much impossible. There is various ways to retrieve output nodes thruogh the API, and all of them are pretty much in aiida-core and provide no way to override them from a plugin. There is also the question of migrating old data, which is essentially not possible through AiiDA's automated migration pathway. If we were to do this, then we would have to provide a script that users run so they can perform the migration manually.

Anyway, as much as I don't like the naming and as much as I dislike having to look at names I don't like, I doubt whether the cost for users downstream would be worth the aesthetic improvement. But if a large portion of the userbase is also in favor of the change, I won't stop it for a new major release.

@mbercx
Copy link
Member Author

mbercx commented Jun 5, 2023

Anyway, as much as I don't like the naming and as much as I dislike having to look at names I don't like, I doubt whether the cost for users downstream would be worth the aesthetic improvement

I definitely see your point. Only making these changes wouldn't be worth the trouble. But I'm also planning to make quite significant backwards-incompatible changes in #952. In general, we should be able to make changes that improve the usability of the code somehow, else we find ourselves always working around these issues.

I'd be happy to work on creating an AST-based approach for updating existing codes and scripts as is done in aiida-upgrade. Doing a manual migration via a script is also a fine approach, although not all backwards-incompatible changes can be migrated this way (e.g. dropping the final SCF). I think this is fine.

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

No branches or pull requests

2 participants