-
Notifications
You must be signed in to change notification settings - Fork 20
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
Naming conventions/design of new library #5
Comments
Another possible source of confusion: |
I have used the following conventions for now (removing
|
Another possible issue:
|
|
As in Side note, they could be returned as a struct instead, to reduce the number of variable names the user has to come up with (with a slight performance penalty in at least older matlab).
That is a bit longer (and implies lengthening other function names), but it could be clearer. I'm not really sure where the preferred length vs clarity balance is.
I could make all the file-level helpers start with |
Probably clarity is more important than length. Returning as a struct seems fine. I do think we should move towards releasing this as it is a clear improvement. |
Just to be clear, some examples of my take on your suggestion:
Note that *_from_template can be cleaner than setting .cdata and using ciftisavereset:
This doesn't create a new variable, or overwrite the original data in |
Ok |
Due to the behavior of |
The current complete list of public function signatures, annotated with read/write and compatibility
dense file extract/replace helpers
file create helpers
diminfo helpers
misc
|
These look good to me |
Do we expect anyone to be confused by |
object instead of file? |
"object" is sort of a compsci jargon term, and is almost equivalent to the word "thing", so it is more of an obfuscation than an explanation. If a name has to be long, it is better if each part of the name conveys some useful information. |
Its always a balance but I'm more for clarity of the function name than file length so long as things are consistent. So if I'm understanding this discussion thread correctly, the prefix "cifti_file_" indicates a function operates on a cifti-file correct? If that's the case then as long as its made clear in the documentation then its fine. I'd also change things like this:
to something like more like:
to indicate the difference between a "cifti" variable in matlab as opposed to a cifti-file that's being operated on. |
That is fine too. |
Actually, the |
Basically, the difficulty in terminology is that |
A previous solution I had hinted at would be to only keep a specifier on the |
Since you asked for comments on the function naming ... For example, what is the role of the Perhaps you already considered this, but could all the |
At present, The create functions mainly allow different extra inputs to set the extra info in a mapping (scalar can take a list of map names, series can take start and step values and a unit specifier). I could make a |
FWIW, I don't find the "file" aspect of the function names all that helpful (which I see was only added by the latest commit). For that matter, I personally think you could probably drop the "diminfo" and "dense" portions of the names as well to actually simplify things. Then you basically have |
I think at least All the
where |
As for the metadata functions, The different To some extent, the "lots of functions" issue is that they can't easily be in a separate folder while still needing only one |
What about making the I'm still not convinced that If some functions are clearly helpers that could be "hidden" in a sub-folder, one way to accomplish that would be to have the main function issue the necessary |
Parcels do not qualify as dense, no, they have entirely different internals and behavior. I'm not sure what additional helpers are likely to be added in the future, so my intuition is to have a specifier in the name to prevent future collision of desired function names. There are not currently helpers for extracting information about parcels, as unlike the common case of expanding dense data back out to full-surface or standard volume format, parcellated data is generally dealt with as-is (and the parcel info is available in the diminfo element as a struct array with one element per parcel that contains lists of vertices and voxels, though those are 0-indexed currently). For the Currently, the main reasons for users to use To put it another way, the |
I'd rather not have the library do an For things the user definitely doesn't need to call, the "private" folder is a matlab special convention for this purpose that does not require any |
That's helpful perspective, although I still think that consolidating the various primarily "helper" functions into a smaller set of "super" functions would simplify the overall library considerably. e.g., for the Related to this consolidation theme, it seems you could perhaps at least consolidate That said, easy for me to suggest all this, as I'm not the one that would be doing the additional dev work and testing. I do think that the current set of proposed functions and their naming is still going to be "intimidating", but this can in principle be managed by a good README. I think I'm about at the limit of what I can meaningfully comment on at a big picture level. |
In terms of possibly some more granular input (e.g., on the organization of specific functions), is there an easy way to "comment" on lines in the code (outside of a change to those lines as part of an existing PR)?
|
Going back to my earlier comment (#5 (comment)), I see that you have a |
It looks like it is possible for wb_command to use "ALL" as the identifier for a single structure (which isn't in the cifti spec, so I should check on that), but even if this weren't the case, using a "special value" to mean "this value isn't specified" can be dangerous, especially for strings. Using advanced logic on the input arguments to make one function behave with 3 or more different behaviors with different mandatory arguments, outputs, or other behavior just doesn't seem like good coding style to me, in any language. I don't expect doing that to increase code reuse much (and reading/editing the code would probably get a lot harder), unlike your good suggestion for the So, at the moment we have 2 votes to keep I opened another issue for discussing the help info formatting, see #7. In the version I am using, matlab's editor doesn't have a "convert selection to caps" option, so I'd prefer to deal with that kind of detail after nailing down the function names. You can comment on lines of code on an existing file, I believe (or at least on lines in an existing commit), but that may be tied to a specific commit (so, easy to lose track of). I deliberately chose to indent the function bodies, as to me it is clearer and more consistent, especially if you define sub-functions in the same file (see private/cifti_parse_xml.m). Every |
Regarding the indentation, your preference is not consistent with matlab's convention, so in that regard, while perhaps more logical to you, it isn't consistent with what others expect for matlab functions (and in that regard, I would argue makes the code less clear to others). In my mind at least, the further consolidation that I was suggesting seems in keeping with typical matlab packages/libraries. The functions in this cifti library strike me as very granular. If that's the design choice, so be it (but the README will be important to orient users). |
I have added a usage section to the readme, see what you think. I disagree that a less-ambiguous coding convention (that makes it easier to spot/avoid errors) is "less clear". It may be unfamiliar to some who have not worked in other languages. Notably, python requires indentation to delineate entire blocks (instead of having something like |
One other comment on the naming: Off the top of my head, I can't really think of other matlab functions/libraries in which the function names themselves attempt to encode info on the type of input (and certainly not to this degree of specificity). Usually, matlab functions are named by what they do, not what they operate on. Here I realize that we have both. Maybe if the info related to the input type was relegated to the end of the function names, so that the action came first...? e.g., The |
When in python, you expect the code to be indented per python's conventions/requirements. For better or worse, matlab has established its own convention (via their own code), and that is not to indent the root level of the function. (I see your point about sub-functions in the same file, but those are generally rare). The current indentation scheme is actually "ambiguous" to me, in that if the entire function doesn't fit on one screen, then per matlab convention, the fact that an entire screen of code is indented is going to send me back in the code searching for the controlling block. It becomes of a matter of which conventions do you adopt, because others will expect them, even if you don't agree with them personally... |
I agree with Mike, but don't see this as a big deal either way... |
Putting classification-type terms first in the function name helps with completion (narrowing down the options without needing to remember the specific verb used for some action), and matlab's editor has tab completion support, though I haven't used it all that much. Of course, at present I would agree that removing Full disclosure: I see enough design decisions in matlab's language and standard functions as questionable that I don't find "base matlab does it this way" to be very compelling on a design perspective. I view that mainly (or even solely) as a familiarity argument. |
You only meant about indentation, I assume? |
Since you raised it, why not To be clear, I'm not taking a position on whether matlab's indentation convention is correct/optimal from a design perspective (that is a flame war sort of thing). Simply that there is a pretty strong convention, and thus I think there is indeed a good familiarity reason for matching that convention. |
I would be fine with I am not inclined to lower my coding standards to that of the average matlab user based on your opinion. Please drop the indentation issue until someone else says it is important to them. |
It's clearly Mathworks own indentation standard, based on how they write their own code. That's why the "average user" writes their matlab functions in the same manner. They are simply emulating Mathworks own standard. With that, I'll drop the issue. |
I think the extended README is helpful. I think it would be helpful to additionally touch base on why "dense" is part of the function names, and why for example there aren't any "parc" (parcellated) analogs at this point in time. |
Such an explanation seems like it has to end up being somewhat silly, here is a draft: The |
It might seem somewhat silly to us, but I think it is a helpful addition for those trying to wrap their head around all this. |
Another option for avoiding I realized a |
I have renamed them to say |
Personally, I find the file names too long (as mentioned before), but they do make sense once you understand the various "fields" within the function names. |
For reference, a new listing of the current function signatures: read/write and compatibility
dense struct extract/replace helpers
struct create helpers and write convenience functions
diminfo helpers
misc
|
I would add that to the end of the README. |
I believe the current API is ready to be a release. |
Given the rewrite of the library, this issue is to gather input on naming conventions, in the hopes of improving things before we declare the new library ready for users.
I see a likely source of confusion in
cifti_dense_get_surface_mapping()
, in that the string "surface mapping" may make users think it refers to "volume to surface mapping", which is a very different thing (resampling, and not merely a file format detail). Anyone have some ideas? A synonym like "correspondence" could work, but that is longer and more clunky, and I am drawing a blank on other good synonyms. Another possibility is to shorten them to "map" instead of "mapping" (cifti_dense_get_vol_all_map()
, etc).It would probably be good to standardize to using only one of
vol
orvolume
in the helper names, but I'm not sure whether clarity or brevity should be favored here.If you have any other comments on the new library other than bugs, you can post them here, too (bugs deserve new issues).
The text was updated successfully, but these errors were encountered: