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

Add copy, split and merge methods to FlattenedStorage #571

Merged
merged 8 commits into from Dec 13, 2021
Merged

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Dec 3, 2021

I've added some methods to split and merge FlattenedStorages that share the same chunk structure. For now I'm deep-copying all the arrays involved for simplicity. If we find later that this is a performance bottleneck we can address that separately.

@pmrv pmrv added the enhancement New feature or request label Dec 3, 2021
@pmrv pmrv requested a review from Leimeroth December 3, 2021 13:18
@Leimeroth
Copy link
Member

I like this, but I am not really sure if it will help me with my data structure. I use different containers for each fit property, but within each container the arrays have the same name, f.e. "target_val", "weight", "fit", ... So if I understand it correctly I can't merge them.

I don't know if it is worth the effort to generalize over my specific use case, since I expect machine learning potential fitting codes to have more similarities with each other than with the classic potential fitting atomicrex does. Maybe an option to prepend a name in front of every array name would help with this. F.e. merging would give array names like "atomic-energy_target_val", "atomic-forces_target_val", etc. But as already said I am not sure if trying to generalize over this is worth it, when every other code only needs a fraction of the arrays and therefore explicit names are the goto solution anyway.

@pmrv
Copy link
Contributor Author

pmrv commented Dec 3, 2021

I think automatic renaming is nice to have. I had actually already planned to model this after pandas.merge, but then forgotten about it again. :/ Patch might only come on Monday though.

@pmrv pmrv added the integration Start the integration tests with pyiron_atomistics/contrib for this PR label Dec 13, 2021
@pmrv pmrv merged commit f8f577e into master Dec 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the split-merge branch December 13, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request integration Start the integration tests with pyiron_atomistics/contrib for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants