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

MemoryReader.timeseries doesn't raise NoDataError on empty AtomGroup, while DCDReader.timeseries does #3898

Closed
hmacdope opened this issue Nov 1, 2022 · 6 comments
Assignees
Labels
Milestone

Comments

@hmacdope
Copy link
Member

hmacdope commented Nov 1, 2022

Expected behavior

The DCDReader timeseries and MemoryReader behave the same.

Actual behavior

The DCDReader raises a NoDataError if an empty atomgroup is provided as the asel argument. This is the same approach taken in #3890

However the MemoryReader just returns an empty array.

Code to reproduce the behavior

import MDAnalysis as mda
from MDAnalysis.tests.datafiles import PSF, DCD,
import numpy as np

u = mda.Universe(PSF, DCD)
u.trajectory.timeseries(u.select_atoms(None)) # raises NoDataError

coords = np.zeros((10,20,3))
u = mda.Universe(coords, format="MEMORY")
u.trajectory.timeseries(u.select_atoms(None)) # returns an empty array. 
@hmacdope hmacdope self-assigned this Nov 1, 2022
@hmacdope hmacdope added the CZI-performance performance track of CZIEOSS4 grant label Nov 1, 2022
@hmacdope hmacdope moved this to In Progress in CZI Performance track Nov 1, 2022
@hmacdope hmacdope added this to the Release 3.0 milestone Nov 1, 2022
@IAlibay
Copy link
Member

IAlibay commented Nov 4, 2022

I believe the recent consensus was that NoDataError was for missing topology attributes rather than zero sized things (see #3837 (comment))

Probably would want some other error type here?

@hmacdope
Copy link
Member Author

hmacdope commented Nov 4, 2022

We could go for ValueError or something, whatever people think is best. Is this best addressed in a seperate PR after #3890 where we homogenise all the call sites?

@IAlibay
Copy link
Member

IAlibay commented Nov 4, 2022

We could go for ValueError or something, whatever people think is best. Is this best addressed in a seperate PR after #3890 where we homogenise all the call sites?

I probably would just advocate for changing DCD's to ValueError and make the other two ValueError? Seems like a simple enough one to have within the scope of #3890?

@orbeckst
Copy link
Member

orbeckst commented Nov 4, 2022

I agree with moving to ValueError.

Thanks for raising the discussion @IAlibay , making the semantics of a historical code base consistent is hard work.

@IAlibay
Copy link
Member

IAlibay commented Nov 4, 2022

We really should document this somewhere, i.e. "Raised when empty input is not allowed or required data are missing." is very non-specific if we're going with a TopologyAttrs centric viewpoint. I'll raise an issue.

@hmacdope
Copy link
Member Author

Fixed in #3890

Repository owner moved this from In Progress to Done in CZI Performance track Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

3 participants