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

Naming of Ops #80

Closed
dietzc opened this issue Oct 29, 2014 · 13 comments
Closed

Naming of Ops #80

dietzc opened this issue Oct 29, 2014 · 13 comments
Assignees
Milestone

Comments

@dietzc
Copy link
Member

dietzc commented Oct 29, 2014

Hi,
we are currently implementing a lot of stuff related to features and we don't know how to actually name the classes. Example: We have an Op (Interface) which calculates the Area for several types (Iterable, Polygon, ...) and we have an Op which calculates the SmallestEnclosingRectangle based on something else.

How would you name the ops?

CalculateSmallestEnclosingRectangle and CalculateArea or rather SmallestEnclosingRectangle and Area. The second option would imply that our Mean would also be called CalculateMean (replace Calculate with whatever you want). However if we would just use SmallestEnclosingRectangle one could think, that this is the class which is the SME itself. In the case of Area, the class also doesn't represent the Area, but for the sake of simplicity and short-names Area seems to be the better choice.

Any ideas @ctrueden @hinerm @dscho @bnorthan ?

Edit: We also have a class CreateHistogram, another case.

@ctrueden
Copy link
Member

We don't use a prefix like calculate for any other ops, so IMO it makes sense to call them area and bounding_box, with Area and BoundingBox interfaces.

@ctrueden
Copy link
Member

Regarding HistogramCreate, we could rename to just Histogram. The iface is Ops.Histogram and the op name is histogram.

Another case of variation in naming is the thresholding stuff, to differentiate between ComputeThreshold and ApplyThreshold ops... but in general, my vote is to keep names shorter.

@dietzc
Copy link
Member Author

dietzc commented Oct 31, 2014

I the case of Histogram you might end up with an Op Histogram which produces a Histogram. So same name, different semantic meaning (I know that we only have Histogram1d at the moment, but there may be cases like the one described above)? I'm also voting for short names, but I'm somehow worried we will have more confusion and exceptions with that naming schema.

@dscho
Copy link
Contributor

dscho commented Nov 6, 2014

Sorry, I wanted to address this today, but I literally spent 7.5h on trying to fix pom-fiji and now I'm spent for the day 😦

@ctrueden
Copy link
Member

ctrueden commented Nov 6, 2014

@dscho I am curious how you wanted to "address" this. Do you mean just write a comment? Or... did you have an idea for changing the code somehow?

@dietzc
Copy link
Member Author

dietzc commented Nov 13, 2014

Any further ideas? I would do the following at the moment: If it is obvious (Area, Mean, Sum, ...) we keep short names. But as soon as there may be some confusion (Histogram vs. Histogram for example), I suggest the prefixes "Calc" or "Compute".

For example the operation would be named
ComputeHistogram
the Histogram itself Histogram.

Any comments?

@ctrueden ctrueden added this to the 0.20.0 milestone Nov 17, 2014
@ctrueden
Copy link
Member

There are two separate issues: what to name the op (i.e., the @Plugin annotation's name value), and what to name the class. These have historically diverged, although we could consider unifying it.

I think this would be good to settle at the next hackathon, so I assigned it to the 0.20.0 milestone.

@ctrueden
Copy link
Member

@dietzc and I discussed and settled on the following:

  • SmallestEnclosingRectangleOp - iface for Op plugins that compute a SmallestEnclosingRectangle (data structure)
  • MeanOp for Op plugins that compute a Mean (data structure)
  • etc.

@ctrueden
Copy link
Member

This issue can be closed out when all ops in imagej-ops abide by this convention.

IdealOutage pushed a commit that referenced this issue Jan 20, 2015
as described in #80
IdealOutage pushed a commit that referenced this issue Jan 20, 2015
IdealOutage pushed a commit that referenced this issue Jan 20, 2015
@ctrueden
Copy link
Member

I really quickly codified some of these naming decisions on the wiki. See also #16.

IdealOutage pushed a commit that referenced this issue Jan 25, 2015
as described in #80
IdealOutage pushed a commit that referenced this issue Jan 25, 2015
IdealOutage pushed a commit that referenced this issue Jan 25, 2015
@ctrueden ctrueden self-assigned this Jan 27, 2015
@dietzc
Copy link
Member Author

dietzc commented Jun 25, 2015

can be closed see wiki https://github.com/imagej/imagej-ops/wiki/Naming

@dietzc dietzc closed this as completed Jun 25, 2015
@ctrueden
Copy link
Member

This issue can be closed out when all ops in imagej-ops abide by this convention.

And we are unfortunately quite far from that: nearly no op ends with the Op suffix mentioned in the documentation. From the docs:

The reason for the Op suffix of FooOp etc. is in case the operation works with a data structure of the same name. For example, a MeanOp might compute a Mean, a SmallestEnclosingRectangleOp might compute a SmallestEnclosingRectangle, and so on.

My understanding was that we would close this issue only after all ops were renamed to conform to this naming scheme.

@ctrueden ctrueden reopened this Jun 25, 2015
@dietzc
Copy link
Member Author

dietzc commented Jun 25, 2015

ah sorry, I overlooked that. makes sense.

@ctrueden ctrueden modified the milestones: 0.16.0, m2 Jul 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants