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

Fix FISTA and ISTA maths in docs #2012

Merged
merged 14 commits into from
Dec 17, 2024
Merged

Fix FISTA and ISTA maths in docs #2012

merged 14 commits into from
Dec 17, 2024

Conversation

lauramurgatroyd
Copy link
Member

@lauramurgatroyd lauramurgatroyd commented Dec 11, 2024

Description

Closes #2013

Changes

Updated FISTA docs:
image

image

Updated ISTA docs:

image

image

Related issues/links

Closes #2013

Checklist

  • I have performed a self-review of my code
  • I have added docstrings in line with the guidance in the developer guide
  • I have updated the relevant documentation
  • I have implemented unit tests that cover any new or modified functionality
  • CHANGELOG.md has been updated with any functionality change
  • Request review from all relevant developers
  • Change pull request label to 'Waiting for review'

Contribution Notes

Please read and adhere to the developer guide and local patterns and conventions.

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License
  • I confirm that the contribution does not violate any intellectual property rights of third parties

@lauramurgatroyd lauramurgatroyd marked this pull request as ready for review December 12, 2024 11:44
Copy link
Member

@MargaretDuff MargaretDuff left a comment

Choose a reason for hiding this comment

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

Thanks Laura!

On lines 40-41 please can we replace "where :math:f is differentiable, :math:g has a simple proximal operator and :math:\alpha^k
is the :code:step_size per iteration."

with

"where :math:f is differentiable, :math:g has a simple proximal operator and :math:\alpha
is the :code:step_size."

and do the same for FISTA, if required.

Wrappers/Python/cil/optimisation/algorithms/FISTA.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/optimisation/algorithms/FISTA.py Outdated Show resolved Hide resolved
lauramurgatroyd and others added 4 commits December 12, 2024 16:13
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Signed-off-by: Laura Murgatroyd <60604372+lauramurgatroyd@users.noreply.github.com>
@lauramurgatroyd
Copy link
Member Author

There's a load of methods we expsose which we shouldnt in the docs so I am going to remove them:
image

@MargaretDuff
Copy link
Member

image
Hi @lauramurgatroyd - looking good now and removing the extra information in FISTA has really helped readability.

Being picky now, please could we rearrange the intro to FISTA/ISTA so it reads:

  • The XXX algorithm is used to solve $$min f(x)+g(x)$$ where f is ... and g is...
  • In each update, the algorithm completes the following step(s)...
  • Remaining notes and definitions

I think this gets the important information over quicker: what the algorithm does and what is required from the functions.

Being very picky, we should also not start a sentence with a mathematical expression. Words like "Where", "For" etc. can be used to start a sentence

@lauramurgatroyd
Copy link
Member Author

image

image

Copy link
Member

@MargaretDuff MargaretDuff left a comment

Choose a reason for hiding this comment

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

@lauramurgatroyd lauramurgatroyd merged commit a47cb15 into master Dec 17, 2024
10 checks passed
@lauramurgatroyd lauramurgatroyd deleted the fista_docs branch December 17, 2024 15:36
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.

Maths in FISTA and ISTA documentation is incorrect/inconsistent
2 participants