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

Update discrete systematic README (Ultrasurface use) #846

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

LeanderFischer
Copy link
Collaborator

The previous version described the use of scripts that aren't part of pisa. This change therefore:

  • removes the pisa unrelated descriptions
  • updates the use instructions with specific config and pipeline example

The previous version described the use of scripts that aren't part of pisa:

* remove the pisa unrelated descriptions
* update the use instructions with specific config and pipeline example
Copy link
Contributor

@thehrh thehrh left a comment

Choose a reason for hiding this comment

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

  • general suggestion: keep documentation focussed on the stage itself to reduce future maintenance cost (import/include syntax and pipeline config examples can already be found elsewhere)
  • other than that, just a few minor (non-critical) remarks

pisa/stages/discr_sys/README.md Outdated Show resolved Hide resolved
pisa/stages/discr_sys/README.md Show resolved Hide resolved
pisa/stages/discr_sys/README.md Outdated Show resolved Hide resolved
pisa/stages/discr_sys/README.md Outdated Show resolved Hide resolved
pisa/stages/discr_sys/README.md Outdated Show resolved Hide resolved
@LeanderFischer
Copy link
Collaborator Author

  • general suggestion: keep documentation focussed on the stage itself to reduce future maintenance cost (import/include syntax and pipeline config examples can already be found elsewhere)

Yes, that's a good point, I removed the import part, but I do think it's valuable to point out at which position this stage should be included in the pipeline order, since it's different to the hypersurface stage.

@LeanderFischer LeanderFischer requested a review from thehrh January 27, 2025 10:32
Copy link
Contributor

@thehrh thehrh left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you. Merging now.

@thehrh thehrh merged commit f63627c into master Jan 27, 2025
2 checks passed
@thehrh thehrh deleted the LeanderFischer-patch-1 branch January 27, 2025 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants