Skip to content

cookiecutterize#75

Merged
grst merged 27 commits into
mainfrom
cookiecutterize
Jun 4, 2024
Merged

cookiecutterize#75
grst merged 27 commits into
mainfrom
cookiecutterize

Conversation

@grst
Copy link
Copy Markdown
Collaborator

@grst grst commented May 22, 2024

Close #66

Migrate pytometry into the scverse-cookiecutter template.

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@grst grst closed this May 22, 2024
@grst grst reopened this May 22, 2024
@grst grst closed this May 22, 2024
@grst grst reopened this May 22, 2024
@grst grst marked this pull request as ready for review May 22, 2024 13:42
@grst
Copy link
Copy Markdown
Collaborator Author

grst commented May 22, 2024

Mostly done. Sphinx doesn't pass because there are some warnings related to the docstrings that need to be resolved.

@grst grst requested review from mbuttner and quentinblampey May 22, 2024 13:43
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Copy Markdown
Collaborator

@mbuttner mbuttner 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 overall - one minor comment on gitignore.

Comment thread .gitignore
Copy link
Copy Markdown
Contributor

@quentinblampey quentinblampey left a comment

Choose a reason for hiding this comment

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

Super nice @grst! Just some minor comments
And the docs still rely on the typehints from the docstrings, I think we should also update this as mentioned in #74 (I can have a look tomorrow)

Comment thread README.md Outdated
Comment thread src/pytometry/pl/_histogram.py Outdated
@grst
Copy link
Copy Markdown
Collaborator Author

grst commented May 23, 2024

I fixed the sphinx warnings by converting the docstrings to numpy-style (with some help of ChatGPT).

Here's a preview of the docs:
https://pytometry--75.org.readthedocs.build/en/75/

@quentinblampey
Copy link
Copy Markdown
Contributor

I pushed some changes:

  • Removed the duplicated default values from the docstrings to show only the ones from the argument's default.
  • Added typing and the right docstrings for some functions that were still missing it
  • normalize_biExp renamed to normalize_biexp, and added an alias for backward compatibility

@mbuttner
Copy link
Copy Markdown
Collaborator

I fixed the sphinx warnings by converting the docstrings to numpy-style (with some help of ChatGPT).

Here's a preview of the docs: https://pytometry--75.org.readthedocs.build/en/75/

This looks pretty amazing already! For the rendered tutorial notebooks, I realized that there is no output shown, yet.

@grst
Copy link
Copy Markdown
Collaborator Author

grst commented May 23, 2024

Did the lamin docs setup execute tutorials upon building the documentation?

With the new setup, this would only work (without extra effort) if the notebooks consume so little memory that they can run on the readthedocs CI.

@mbuttner
Copy link
Copy Markdown
Collaborator

Yes, we made the separation between "examples and tasks" (not executed) and "tutorials", which are executed, but are designed to consume little memory.

@grst
Copy link
Copy Markdown
Collaborator Author

grst commented May 23, 2024

ok, let's try

@grst grst force-pushed the cookiecutterize branch from fb71191 to 39ef51f Compare May 24, 2024 11:14
@grst
Copy link
Copy Markdown
Collaborator Author

grst commented May 24, 2024

Seems to have worked now :)

@quentinblampey, the scatterplot in the tutorial looks a bit strange. I suspect it's due to xlim being provided
image

@quentinblampey
Copy link
Copy Markdown
Contributor

Nice catch @grst, indeed on this example the minimal value was -4233164.5, therefore a lot of bins were used for negative numbers, while not shown afterwards
I fixed it by setting range in np.histogram2d

@Zethson
Copy link
Copy Markdown
Member

Zethson commented May 24, 2024

We use

at lamin with pre-commit to ensure that no executed notebooks are pushed. I think this setup here wants that, right?

@grst
Copy link
Copy Markdown
Collaborator Author

grst commented May 24, 2024

Yes, but only for the notebooks in tutorials not examples. But it would be only to ensure the repo stays clean. With the latest changes, the notebooks in tutorials will be executed either way.

@quentinblampey
Copy link
Copy Markdown
Contributor

Does it look good for everyone?

@grst grst merged commit ccab5c0 into main Jun 4, 2024
@grst grst deleted the cookiecutterize branch June 4, 2024 09:18
@grst
Copy link
Copy Markdown
Collaborator Author

grst commented Jun 4, 2024

@mbuttner, is it possible to forward to https://pytometry.readthedocs.io/en/latest/index.html from the old netlify site?

@mbuttner
Copy link
Copy Markdown
Collaborator

mbuttner commented Jun 4, 2024

Yes, that's possible, see https://docs.netlify.com/configure-builds/file-based-configuration/#redirects
However, this means we have to rebuild the netlify page.

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.

Cookiecutterize

4 participants