Skip to content

Conversation

@hinerm
Copy link
Member

@hinerm hinerm commented Jul 31, 2024

These changes restructure the introductory sections of the read the docs pages to get users into Ops use relatively smoothly (hopefully) by tuning the Getting Started section to be more of a "quick start" into scripting, while focusing and fleshing out the "how-to guides" section, e.g. by prioritizing finding Ops before calling them and adding missing information on Functions and Inplaces.

hinerm added 4 commits July 29, 2024 16:02
- Put Fiji in the quick start section since this is how we expect people
  to interact with Ops
- Focus the how-to guides to cover usage in-depth
- Move benchmarks to Development since because of the technical scope of
  this section
Add some info on creating an OpEnvironment and add more words to clarify
API usage.
- Expand to cover Function and InPlace options
- Reword for clarity
@hinerm hinerm requested review from elevans and gselzer July 31, 2024 18:27
Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hinerm This is super great! Much friendlier! 🤩

I pushed some commits with minor adjustments on top of your new prose, and I think we're good to merge!

Note that although the final method call changes for each mode of operation, *this is based on the path taken through the `OpBuilder` chain*. For example, we cannot call the `compute()` method if we haven't provided an `.output()`:

```
# Does not compute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😂

```text
filter.gauss:
- (input, sigmas, @CONTAINER container1) -> None
- (input, sigmas, outOfBounds = null, @CONTAINER container1) -> None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not part of this PR I know, but this output mixes nulls and Nones... we should make that more consistent.

@ctrueden ctrueden merged commit 3b3f3b2 into main Jul 31, 2024
@ctrueden ctrueden deleted the quick-start branch July 31, 2024 19:29
Copy link
Member

@gselzer gselzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that @ctrueden merged before I got the chance to finish my review, but we can still think about my points if we want! None are really important!

With the name established in the `OpBuilder` chain, we can then specify our input(s) with the `.input()` method.

For our gaussian blur, we will pass as inputs our input image `inImage`, and a `double` as our sigma parameter:
For this Gaussian blur, we have two inputs: `inImage` is the image we want to blur, and a `double` value as our sigma parameter:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For this Gaussian blur, we have two inputs: `inImage` is the image we want to blur, and a `double` value as our sigma parameter:
For this Gaussian blur, we have two inputs: `inImage` is the image we want to blur, and a `double` value as the standard deviation of the smoothing function:

What do you think about using the technical term?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the simpler our sigma parameter 😉

var outImage = ops.op("filter.gauss").input(inImage, 2.0).apply()
```

*Inplaces* are used when we want to destructively modify one of the existing inputs (which is explicitly forbidden by *computers*; a *computer* Op's output should be a different object from all of its inputs). We indicate this by the `mutate#()` method, where the `#` corresponds to the *parameter index* that will be modified:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be confusing, or clarifying, to note this is 1-indexed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Note that the default `OpEnvironment` implementations cache Op requests* - this means that repeated `OpBuilder` requests targeting the same action will be faster than the original matching call.

## Additions: Matching with classes
### Matching with classes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is classes better or worse than types?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ctrueden
Copy link
Member

Sorry for the overlap @gselzer. I filed #250 to address your comments.

ctrueden added a commit that referenced this pull request Jul 31, 2024
Make further updates based on feedback from PR #249
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

Successfully merging this pull request may close these issues.

4 participants