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

beef up FAI detection related parameter explanations – #110 #143

Merged
merged 7 commits into from
Dec 5, 2024

Conversation

jGaboardi
Copy link
Collaborator

@jGaboardi jGaboardi self-assigned this Dec 5, 2024
@jGaboardi jGaboardi added the documentation Improvements or additions to documentation label Dec 5, 2024
@anastassiavybornova
Copy link
Collaborator

@jGaboardi thanks for the deciphering!! i did a second pass on the parameter documentation - but now I think it needs a third 😅 as in: it is factually correct now but potentially too detailed / verbose

Copy link
Collaborator

@anastassiavybornova anastassiavybornova left a comment

Choose a reason for hiding this comment

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

(see my commit & comment above)

@martinfleis
Copy link
Contributor

I'm fine with @anastassiavybornova version.

@jGaboardi
Copy link
Collaborator Author

Do not merge. Still reviewing.

@martinfleis
Copy link
Contributor

Won't, also due to

I'll cascade them into internal functions prior to merging.

Copy link
Collaborator Author

@jGaboardi jGaboardi left a comment

Choose a reason for hiding this comment

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

Very nice @anastassiavybornova . Small language changes and little typos only.

neatnet/simplify.py Outdated Show resolved Hide resolved
neatnet/simplify.py Outdated Show resolved Hide resolved
neatnet/simplify.py Outdated Show resolved Hide resolved
neatnet/simplify.py Show resolved Hide resolved
neatnet/simplify.py Show resolved Hide resolved
@jGaboardi
Copy link
Collaborator Author

@anastassiavybornova Would you like to review my suggestions or shall I commit them myself? (still need to update the other docstrings after that).

anastassiavybornova and others added 3 commits December 5, 2024 15:47
Co-authored-by: James Gaboardi <jgaboardi@gmail.com>
Co-authored-by: James Gaboardi <jgaboardi@gmail.com>
Co-authored-by: James Gaboardi <jgaboardi@gmail.com>
@anastassiavybornova
Copy link
Collaborator

@jGaboardi reviewing them now!

@anastassiavybornova
Copy link
Collaborator

@jGaboardi done

@jGaboardi
Copy link
Collaborator Author

I'll merge this once CI is green.

@jGaboardi jGaboardi merged commit bb03dd1 into main Dec 5, 2024
12 checks passed
@jGaboardi jGaboardi deleted the GH110_explain_threshold_params branch December 5, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

more explanation for *_threshold_* params in simply.simplify_network()
3 participants