{"action":"created","issue":{"url":"https://api.github.com/repos/Amber-MD/cpptraj/issues/22","labels_url":"https://api.github.com/repos/Amber-MD/cpptraj/issues/22/labels{/name}","comments_url":"https://api.github.com/repos/Amber-MD/cpptraj/issues/22/comments","events_url":"https://api.github.com/repos/Amber-MD/cpptraj/issues/22/events","html_url":"https://github.com/Amber-MD/cpptraj/pull/22","id":97646472,"number":22,"title":"add more constructor for Frame","user":{"login":"hainm","id":4451957,"avatar_url":"https://avatars.githubusercontent.com/u/4451957?v=3","gravatar_id":"","url":"https://api.github.com/users/hainm","html_url":"https://github.com/hainm","followers_url":"https://api.github.com/users/hainm/followers","following_url":"https://api.github.com/users/hainm/following{/other_user}","gists_url":"https://api.github.com/users/hainm/gists{/gist_id}","starred_url":"https://api.github.com/users/hainm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hainm/subscriptions","organizations_url":"https://api.github.com/users/hainm/orgs","repos_url":"https://api.github.com/users/hainm/repos","events_url":"https://api.github.com/users/hainm/events{/privacy}","received_events_url":"https://api.github.com/users/hainm/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":13,"created_at":"2015-07-28T08:32:57Z","updated_at":"2015-07-28T23:39:21Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/Amber-MD/cpptraj/pulls/22","html_url":"https://github.com/Amber-MD/cpptraj/pull/22","diff_url":"https://github.com/Amber-MD/cpptraj/pull/22.diff","patch_url":"https://github.com/Amber-MD/cpptraj/pull/22.patch"},"body":"@drroe \r\n\r\nSo I \"officially\" create this PR to add one more constructor for Frame. (as we discussed in g-doc a while ago). \r\n\r\n**Motivation**: current implementation of cpptraj require coordinates copied twice when transferring from disk to in-memory numpy array. `cpptraj` read coords from disk to Frame class, then pytraj copy frame's coordinates to numpy (two copies need to be done to convert `discontiguous memory` (vector of frames) to contiguous memory block in numpy).\r\n\r\n* I think it's not ok to do this for very large system (may be > 100K atoms).  So I add this constructor to use Frame just as a pointer to a **pre-allocated** numpy array and instruct cpptraj to read coords directly to numpy memory block. Memory is not own by cpptraj but numpy (controlled through `pytraj`).\r\n\r\nSo this constructor is just designed for cpptraj/pytraj interface.\r\nThis implementation 2 times speeds up + memory saving.\r\n\r\n* Secondly, this is also speed up the frame slicing of numpy array. Current implementation needs to slice numpy array, convert to Frame object so cpptraj can understand. If we use constructor above, no data is copied. Of course, this only good for cpptraj's actions that does not change mem location. \r\n\r\nlet me know if it's ok to merge.\r\n\r\nHai"},"comment":{"url":"https://api.github.com/repos/Amber-MD/cpptraj/issues/comments/125782736","html_url":"https://github.com/Amber-MD/cpptraj/pull/22#issuecomment-125782736","issue_url":"https://api.github.com/repos/Amber-MD/cpptraj/issues/22","id":125782736,"user":{"login":"drroe","id":10144183,"avatar_url":"https://avatars.githubusercontent.com/u/10144183?v=3","gravatar_id":"","url":"https://api.github.com/users/drroe","html_url":"https://github.com/drroe","followers_url":"https://api.github.com/users/drroe/followers","following_url":"https://api.github.com/users/drroe/following{/other_user}","gists_url":"https://api.github.com/users/drroe/gists{/gist_id}","starred_url":"https://api.github.com/users/drroe/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/drroe/subscriptions","organizations_url":"https://api.github.com/users/drroe/orgs","repos_url":"https://api.github.com/users/drroe/repos","events_url":"https://api.github.com/users/drroe/events{/privacy}","received_events_url":"https://api.github.com/users/drroe/received_events","type":"User","site_admin":false},"created_at":"2015-07-28T23:39:21Z","updated_at":"2015-07-28T23:39:21Z","body":"@hainm This still isn't really addressing the heart of the matter: if Frame doesn't own the data it should *not* attempt to free it, i.e. if memory allocation occurs outside of Frame, destruction should as well. I think it's better to do something like this:\r\n```C++\r\nFrame::Frame(int natomIn, const double* xIn) : \r\n  natom_(natomIn),\r\n  maxnatom_(natomIn),\r\n  ncoord_(natomIn*3), \r\n  T_(0.0),\r\n  time_(0.0),\r\n  X_(xIn),\r\n  V_(0),\r\n  Mass_(natomIn, 1.0),\r\n  freeMemory_(false)\r\n{ ... }\r\n\r\nFrame::~Frame() {\r\n  if (freeMemory_) {\r\n    if (X_ != 0) delete[] X_;\r\n    if (V_ != 0) delete[] V_;\r\n  }\r\n}\r\n```\r\nThen the other constructors get freeMemory_ set to true. Make sure you update the copy constructor, swap function, and assignment operator as well. All the SetupFrame... routines should probably be protected as well. This is a semi-large change to the intent of Frame so we need to be careful."}}