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

Use FlattenedStorage in NeighborsTrajectory #387

Merged
merged 11 commits into from
Nov 26, 2021
Merged

Use FlattenedStorage in NeighborsTrajectory #387

merged 11 commits into from
Nov 26, 2021

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Oct 7, 2021

This also allows it to be used with grand-canonical trajectories. Has some small fixes for DataContainer compat too, see commit messages.

@pmrv pmrv added the enhancement New feature or request label Oct 7, 2021
Copy link
Member

@sudarsan-surendralal sudarsan-surendralal left a comment

Choose a reason for hiding this comment

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

This looks good to me. If possible, can you also add a test to check if the shapes are consistent?

@pmrv
Copy link
Contributor Author

pmrv commented Oct 8, 2021

I realize now that this will change the shape of the attributes neighbor_indices, etc. because you can't put them in full numpy arrays anymore. (Actually right now the code stores the flat arrays, but that probably breaks you old code).

What if I make it so that the property methods check whether the trajectory canonical or grand-canonical and in the latter case return full arrays with filler values (basically like @samwaseda does it for the neighbor class). Then old code won't break and new code that wants to deal with grand-canonical trajectories is better off dealing with the underlying FlattenedStorage anyway.

@stale
Copy link

stale bot commented Oct 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Copy link
Member

@sudarsan-surendralal sudarsan-surendralal left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@sudarsan-surendralal sudarsan-surendralal left a comment

Choose a reason for hiding this comment

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

Oops one of the tests fail

AttributeError: 'float' object has no attribute 'sqrt'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/runner/work/pyiron_atomistics/pyiron_atomistics/tests/lammps/test_base.py", line 412, in test_dump_parser_water
    self.assertTrue(np.allclose(np.linalg.norm(neigh_traj_obj.vecs, axis=-1),
  File "<__array_function__ internals>", line 5, in norm
  File "/usr/share/miniconda/envs/test/lib/python3.9/site-packages/numpy/linalg/linalg.py", line 2561, in norm
    return sqrt(add.reduce(s, axis=axis, keepdims=keepdims))
TypeError: loop of ufunc does not support argument 0 of type float which has no callable sqrt method

@pmrv
Copy link
Contributor Author

pmrv commented Nov 3, 2021

Seems it's a bug in FlattenedStorage, will fix in base.

Enables the class to be used for grand-canonical trajectories as well.
This is necessary to allow correct loading from HDF.
This makes sure, it will be correctly saved/loaded to/from HDF.
When a predefined store is used, make sure to call add_chunk only when
there's more structures in has_structure then chunks already exist in
store.  Assuming that the given store already holds some information
about the structures in has_structure, this ensures that the neighbor
information lines up with that.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1507327506

  • 17 of 20 (85.0%) changed or added relevant lines in 2 files are covered.
  • 50 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 69.782%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/atomistics/structure/neighbors.py 16 19 84.21%
Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/atomistics/job/atomistic.py 50 72.87%
Totals Coverage Status
Change from base Build 1500854841: -0.02%
Covered Lines: 11438
Relevant Lines: 16391

💛 - Coveralls

@pmrv pmrv added the integration Start the notebook integration tests for this PR label Nov 26, 2021
@pmrv pmrv merged commit 788d9c7 into master Nov 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the flattraj branch November 26, 2021 13:58
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 notebook integration tests for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants