Conversation
…torial New docs/source/effects/ section covering: - Overview of all built-in effect types, the simulation pipeline, z-order system, YAML configuration, and runtime interaction - Tutorial on creating custom effects with a non-symmetric vignetting flat field example, plus guidance on sharing/contributing effects Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use flowchart LR with <br/> for line breaks so the three pipeline phases render side by side - Move Effect Categories to the end of the page - Add a note box in YAML Configuration pointing to the IRDB for real-world examples Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #899 +/- ##
=======================================
Coverage 74.76% 74.76%
=======================================
Files 69 69
Lines 8936 8936
=======================================
Hits 6681 6681
Misses 2255 2255 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
A test to see whether it's worth get claude to work a bit on the documentation, given that we have two science meetings coming up in 6-7 weeks |
There was a problem hiding this comment.
I have my issues with this. Knowing that this was the toaster's work and not your own, I won't hold back on the criticism.
Most importantly in the broader scope, I don't think it's a good point in time to document the inner details of the effects, when those are about to change. What do I mean by change? Well, we outlined it more than two years ago at this point in #387. While I don't have time to do it now, I do think it will be done before the year is over. But I know your answer, we can always come back and change the text here, or have it changed by the toaster.
More specifically, we should be VERY careful what we write about the z-order! As we know, only the "hundreds digit" is considered by ScopeSim, the final two digits are completely ignored. When first discovered, we treated this as a bug and tried to fix it, but it turns out letting the final order in which effects are applied be defined in the YAML order allows for much better control in specific cases. In any case, z_order will be phased out before v1.0. If we really need this docs section now, we should strongly clarify how this parameter is actually handled by ScopeSim and mention that an alternative it being worked on.
Similarly for the "custom effects" page, I had for a long time now that plan to enable simply callables to be passed as simple custom effects, but alas I haven't gotten around to that so far either. I don't think we currently have the person-power to handle any PRs with new effect subclasses from outside, so I'm not sure inviting people to do that is necessarily a good idea at this point. Maybe later. Also, don't tell users to modify anything in their local ScopeSim installation. That just bad practice and asking for weird bugs that they then bother us with, because they broke their own environment. It happened last autumn. Please remove that subsection.
Also, please before clicking the "request review" button, please check yourself that YOU would be ready to merge this. A review request is not a collaboration invitation on a non-finished PR, but it's saying "here's my work, I consider it finished, please let me know if I missed anything". What I'm referring to?
- The CI tripped on two of the hyperlinks on those two pages, which are incorrect. This should have caught your attention.
- The mermaid flowchart doesn't render as intended and is not useable in it's current form, see individual comment.
I added much more in the individual review comments, let's continue there...
I originally put a paragraph here about em dashes, but have since decided to just replace those. I think I missed at least two of them, but whatever. Ignore.
| flowchart LR | ||
| subgraph Setup ["Setup Phase"] | ||
| direction TB |
There was a problem hiding this comment.
Now, I'm assuming it was smart enough to understand that all of this flowchart doesn't fit the page width, and thus tried to get the individual parts to be displayed top-to-bottom (direction TB), but the overall flowchart left-to-right (flowchart LR). Unfortunately, that's not the way it renders on RTD. I'm getting the impression you didn't even check that before requesting my review, but whatever. I'm not sure what's causing it to not render in the intended way, but I think it's fine to just display the whole thing top-to-bottom. But certainly needs some more work, because the way it renders now on RTD (on my normal laptop screen) it's illegible.
| | Z-Order Range | Pipeline Stage | Applied To | OpticsManager Property | | ||
| |:---:|---|---|---| | ||
| | 200–299 | FOV setup | `FovVolumeList` | `fov_setup_effects` | |
There was a problem hiding this comment.
Now, I know this renders fine as a table. Call me a boomer, but I prefer it to also look nice in raw markdown. In other words, please format the markdown table so it looks like an actual table. Takes two minutes if done manually.
| Effects are typically defined in YAML instrument packages. Each effect entry | ||
| specifies the class name and configuration parameters: |
There was a problem hiding this comment.
If we're explaning the YAML syntax for effects, I think two important aspects are missing: Firstly, the fact that the name parameters is the name that's used to identify that effect later in the optical train. Secondly, less critical but still worth mentioning, that the description parameter only exists as a kind of explanatory comment inside the yaml file. It is utterly ignored by the YAML parser and does not show up anywhere in ScopeSim. You could even call it redundant, if it wasn't for the purpose of making the YAML definition easier to understand.
| ```yaml | ||
| effects: | ||
| - name: detector_qe_curve | ||
| description: Quantum efficiency of the battery of detectors |
There was a problem hiding this comment.
I don't understand what a "battery of detectors" is supposed to be in this context.
| - [Turning Effects on or off](../5_liners/effects_include.md) | ||
| - [Using bang strings and hash strings](../5_liners/bang_strings.md) | ||
|
|
||
| ## Effect Categories |
There was a problem hiding this comment.
I agree that this overview is useful, but we already have it, e.g. here for TER curves (and other subpages for each module)! I'm always against duplicating information, but in this case specifically, because we will certainly forget to modify this list if we change / add / remove an effect. In my (not so humble) opinion, this whole section should be removed and instead readers should be kindly pointed here: _autosummary/scopesim.effects.html
| describing your effect and sharing the code. The ScopeSim team can help | ||
| integrate it into the package. |
There was a problem hiding this comment.
We don't have time for this, but whatever. Let's just hope nobody takes this as an invitation...
|
Further note on Claude, which I've seen a few times now in our team in the last couple weeks: I hate how it's generating commit messages where the initial line is too long to fit on one line. I don't know if this can be configured in Claude's settings in any way, but if yes the pleeaaase do that! Looking at all of your who use it. For reference: Anything that's more than 70 (or 71, but NOT 80) characters is broken into two lines in GitHub's UI. This makes it obnoxious to look at, in the PR and afterwards, especially in the context of e.g. git blame. Please, whether done manually or by the AI, always keep your first commit message line within 70 characters, than if there needs to be more explanation, add a blank line and then do whatever you want (anything after that blank line can be longer, no problem). |
Co-authored-by: Fabian H. <73600109+teutoburg@users.noreply.github.com>
2 participants
Summary
docs/source/effects/documentation section with two pages:index.rsttoctreeTest plan
5_liners/,examples/) resolve🤖 Generated with Claude Code