{"action":"created","issue":{"url":"https://api.github.com/repos/MRtrix3/mrtrix3/issues/225","labels_url":"https://api.github.com/repos/MRtrix3/mrtrix3/issues/225/labels{/name}","comments_url":"https://api.github.com/repos/MRtrix3/mrtrix3/issues/225/comments","events_url":"https://api.github.com/repos/MRtrix3/mrtrix3/issues/225/events","html_url":"https://github.com/MRtrix3/mrtrix3/pull/225","id":69963852,"number":225,"title":"Updated syntax for Image classes ","user":{"login":"jdtournier","id":3221000,"avatar_url":"https://avatars.githubusercontent.com/u/3221000?v=3","gravatar_id":"","url":"https://api.github.com/users/jdtournier","html_url":"https://github.com/jdtournier","followers_url":"https://api.github.com/users/jdtournier/followers","following_url":"https://api.github.com/users/jdtournier/following{/other_user}","gists_url":"https://api.github.com/users/jdtournier/gists{/gist_id}","starred_url":"https://api.github.com/users/jdtournier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jdtournier/subscriptions","organizations_url":"https://api.github.com/users/jdtournier/orgs","repos_url":"https://api.github.com/users/jdtournier/repos","events_url":"https://api.github.com/users/jdtournier/events{/privacy}","received_events_url":"https://api.github.com/users/jdtournier/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/MRtrix3/mrtrix3/labels/critical","name":"critical","color":"e11d21"},{"url":"https://api.github.com/repos/MRtrix3/mrtrix3/labels/enhancement","name":"enhancement","color":"84b6eb"},{"url":"https://api.github.com/repos/MRtrix3/mrtrix3/labels/help%20wanted","name":"help wanted","color":"159818"}],"state":"open","locked":false,"assignee":{"login":"jdtournier","id":3221000,"avatar_url":"https://avatars.githubusercontent.com/u/3221000?v=3","gravatar_id":"","url":"https://api.github.com/users/jdtournier","html_url":"https://github.com/jdtournier","followers_url":"https://api.github.com/users/jdtournier/followers","following_url":"https://api.github.com/users/jdtournier/following{/other_user}","gists_url":"https://api.github.com/users/jdtournier/gists{/gist_id}","starred_url":"https://api.github.com/users/jdtournier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jdtournier/subscriptions","organizations_url":"https://api.github.com/users/jdtournier/orgs","repos_url":"https://api.github.com/users/jdtournier/repos","events_url":"https://api.github.com/users/jdtournier/events{/privacy}","received_events_url":"https://api.github.com/users/jdtournier/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/MRtrix3/mrtrix3/milestones/1","html_url":"https://github.com/MRtrix3/mrtrix3/milestones/MRtrix3%203.0%20release%20for%20ISMRM","labels_url":"https://api.github.com/repos/MRtrix3/mrtrix3/milestones/1/labels","id":1036648,"number":1,"title":"MRtrix3 3.0 release for ISMRM","description":"I think it's time we got a proper release going - we've been in beta for a year now... It would be great to have a proper 3.0 release come ISMRM. So let's set a Milestone, and try to get things in order by then.\r\n\r\nTo get this going, we should go through the list of current issues and identify those that need to be addressed by that stage. It would also be good to identify applications that might be ready for inclusion into the MRtrix3 core by that stage - I'm thinking multi-tissue CSD, SIFT2, AFD, rcsdeconv, etc. Feel free to chip in on this issue, or better yet, create an issue and link it with this milestone...\r\n\r\nCheers,\r\nDonald.","creator":{"login":"jdtournier","id":3221000,"avatar_url":"https://avatars.githubusercontent.com/u/3221000?v=3","gravatar_id":"","url":"https://api.github.com/users/jdtournier","html_url":"https://github.com/jdtournier","followers_url":"https://api.github.com/users/jdtournier/followers","following_url":"https://api.github.com/users/jdtournier/following{/other_user}","gists_url":"https://api.github.com/users/jdtournier/gists{/gist_id}","starred_url":"https://api.github.com/users/jdtournier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jdtournier/subscriptions","organizations_url":"https://api.github.com/users/jdtournier/orgs","repos_url":"https://api.github.com/users/jdtournier/repos","events_url":"https://api.github.com/users/jdtournier/events{/privacy}","received_events_url":"https://api.github.com/users/jdtournier/received_events","type":"User","site_admin":false},"open_issues":24,"closed_issues":51,"state":"open","created_at":"2015-03-24T16:09:24Z","updated_at":"2015-08-12T21:54:35Z","due_on":"2015-05-30T23:00:00Z","closed_at":null},"comments":66,"created_at":"2015-04-21T22:06:14Z","updated_at":"2015-08-14T16:50:04Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/MRtrix3/mrtrix3/pulls/225","html_url":"https://github.com/MRtrix3/mrtrix3/pull/225","diff_url":"https://github.com/MRtrix3/mrtrix3/pull/225.diff","patch_url":"https://github.com/MRtrix3/mrtrix3/pull/225.patch"},"body":"This branch introduces a new cleaner API for the Image classes, as discussed in #211 \r\n\r\nCurrently, the only command working is `bogus` - and even then it only works on some inputs. Still need to implement preloading and scratch buffer support. A long way to go yet...\r\n\r\nBut it does make things look a bit cleaner. For example, from `cmd/bogus.cpp`:\r\n\r\n```C++\r\nvoid run ()\r\n{\r\n  value_type power = 2.0;\r\n  auto opt = get_options (\"power\");\r\n  if (opt.size())\r\n    power = opt[0][0];\r\n\r\n  auto in = Header::open (argument[0]).get_image<value_type>();\r\n  auto header = in.header();\r\n  header.datatype() = DataType::Float32;\r\n  auto out = Header::create (argument[1], header).get_image<value_type>(); \r\n\r\n  ThreadedLoop (in).run ([&](decltype(in)& vin, decltype(out)& vout) {\r\n    vout.value() = std::pow (vin.value(), power);\r\n    }, in, out);\r\n}\r\n```\r\n\r\nAnyway, not ready to merge by any stretch, but thought I'd give people a sneak preview :smile: \r\n\r\nOnce it's stable, I wouldn't mind some help porting all the apps to the new framework..."},"comment":{"url":"https://api.github.com/repos/MRtrix3/mrtrix3/issues/comments/131177522","html_url":"https://github.com/MRtrix3/mrtrix3/pull/225#issuecomment-131177522","issue_url":"https://api.github.com/repos/MRtrix3/mrtrix3/issues/225","id":131177522,"user":{"login":"jdtournier","id":3221000,"avatar_url":"https://avatars.githubusercontent.com/u/3221000?v=3","gravatar_id":"","url":"https://api.github.com/users/jdtournier","html_url":"https://github.com/jdtournier","followers_url":"https://api.github.com/users/jdtournier/followers","following_url":"https://api.github.com/users/jdtournier/following{/other_user}","gists_url":"https://api.github.com/users/jdtournier/gists{/gist_id}","starred_url":"https://api.github.com/users/jdtournier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jdtournier/subscriptions","organizations_url":"https://api.github.com/users/jdtournier/orgs","repos_url":"https://api.github.com/users/jdtournier/repos","events_url":"https://api.github.com/users/jdtournier/events{/privacy}","received_events_url":"https://api.github.com/users/jdtournier/received_events","type":"User","site_admin":false},"created_at":"2015-08-14T16:50:04Z","updated_at":"2015-08-14T16:50:04Z","body":"> I never suggested that I wanted to have a set_index or set_value (I was arguing for keeping index as is). A agree it would be a pain to have to type image.set_value (image.value() * 2). On the topic of image.index(axis), I know you have already removed the [] operator, however this would be my preferred way to change the image index (providing it was well documented). This would allow things like assign_pos_of (vec).to (image) where vec is any vector-like object with operator[] access. However, I know we agreed that .index(axis) was more explicit, so I'm happy to keep things as is...just putting it out there.\r\n\r\nGlad to hear it! I can always re-introduce the operator[] methods if you really want them, but the `assign_pos_of()` methods already work for vector-like objects :wink: \r\n\r\n> On that note, I think we should write Helper functions for set_ndim and set_DW_scheme to keep things consistent.\r\n\r\nOK, can do.\r\n\r\n> With respect to wasted effort in computing these on load, or recomputing when changing the voxel spacing or image2scanner transform. I think that is a moot point. Compared to the number of CPU cycles in a typical MRtrix command, a couple of matrix operations are irrelevant. The spacing is also rarely changed after an image is loaded (I can't think of any cases off the top of my head). Same with updating the image transform. Than again, you do use bit masks in mrview displayable objects instead bools just to save a couple of bytes :smirk:.\r\n\r\nIt's not so much on load that it's a problem - any header modification would trigger this, even on a temporary copy. But then I just twigged that you might be talking about making the transform class a member of the Image class? In which case that would work - by the time the Image is formed, it's implicitly finalised anyway. The problem with that approach though is that you'd then have a different interface for Header than for Image, which is problematic given how often the two are used interchangeably in template utility functions. What we could do is provide an _additional_ Transform class for the Image & Adapter classes, but keep the existing `transform()` method as it is - although we'd probably want to rename it by that point. \r\n\r\nOtherwise, if we have to implement it at the Header level, then every time you modify a temporary Header's voxel sizes or transform, you'd trigger the update. If you were to modify the voxel sizes (for example if you're regridding, you might copy the input header, then modify to match a template or a user-supplied set of voxel sizes), then you'd trigger a minimum of 4 updates. I appreciate it's not a big deal CPU-wise, it just jars with my general minimum-effort approach to life...\r\n\r\n> With regards to keeping the Header class a pure representation of what is stored on disk, I'm not sold on that one either. I think the API should be as convenient as possible for the user, so they can write as few lines as possible. Even if this means that we do a bit of extra work for them. For example take the example of warpinit in the updated_syntax branch. If we included the voxel2scanner transform in the header then the entire write_coordinates class could be replaced with a 1 line lamba function.\r\n\r\nI kind of agree and kind of not. In my experience, providing a nice comfortable interface by abstracting too many details can actually get in the way. I'd rather provide a very direct representation of the data with minimal changes to what was stored, with convenience functions that do provide the kind of comfortable API you're talking about. For example, we're already talking about rolling back just this kind of convenience massaging of data in #314, and I've also removed a lot of the upfront sanitisation of the DW scheme that was previously done automatically, precisely because in some edge cases, it gets in the way. And I've also had to prevent the automatic sanitisation of the transform when creating images (issue 74 on Gitlab), as it causes issued when transforming images. \r\n\r\nBut that's not to say that we can't therefore provide direct access to all the transformation matrices, just that I don't want to prioritise convenience at the expense of access to the original information. Or to put it another way: I want the _data structures_ to remain as close to the original data as possible, while providing _methods_ or _functions_ that provide a convenient and simple API. It's a subtle difference, but it distinguishes more clearly between what the original data was, and at what point it gets modified by MRtrix for convenient use.\r\n\r\nSo, to summarise my opinion on this matter: I'm happy to add these matrices to the Image and Adapter classes, since these are by definition finalised once instantiated. This gets around my aversion to automatic update on voxel size modification... And means the Header remains lean and as close to the original data as possible. The simplest is probably to add a shared_ptr to a Transform class to store this. And we could rename the current `transform()` method to avoid clashes with the new API. How does that sound...? "}}